diff mbox series

[v2] i2c: piix4: Fix adapter not be removed in piix4_remove()

Message ID 20221027121353.181695-1-chenzhongjin@huawei.com
State New
Headers
Series [v2] i2c: piix4: Fix adapter not be removed in piix4_remove() |

Commit Message

Chen Zhongjin Oct. 27, 2022, 12:13 p.m. UTC 
  In piix4_probe(), the piix4 adapter will be registered in:

   piix4_probe()
     piix4_add_adapters_sb800() / piix4_add_adapter()
       i2c_add_adapter()

Based on the probed device type, piix4_add_adapters_sb800() or single
piix4_add_adapter() will be called.
For the former case, piix4_adapter_count is set as the number of adapters,
while for antoher case it is not set and kept default *zero*.

When piix4 is removed, piix4_remove() removes the adapters added in
piix4_probe(), basing on the piix4_adapter_count value.
Because the count is zero for the single adapter case, the adapter won't
be removed and makes the sources allocated for adapter leaked, such as
the i2c client and device.

These sources can still be accessed by i2c or bus and cause problems.
An easily reproduced case is that if a new adapter is registered, i2c
will get the leaked adapter and try to call smbus_algorithm, which was
already freed:

Triggered by: rmmod i2c_piix4 && modprobe max31730

 BUG: unable to handle page fault for address: ffffffffc053d860
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 Oops: 0000 [#1] PREEMPT SMP KASAN
 CPU: 0 PID: 3752 Comm: modprobe Tainted: G
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 RIP: 0010:i2c_default_probe (drivers/i2c/i2c-core-base.c:2259) i2c_core
 RSP: 0018:ffff888107477710 EFLAGS: 00000246
 ...
 <TASK>
  i2c_detect (drivers/i2c/i2c-core-base.c:2302) i2c_core
  __process_new_driver (drivers/i2c/i2c-core-base.c:1336) i2c_core
  bus_for_each_dev (drivers/base/bus.c:301)
  i2c_for_each_dev (drivers/i2c/i2c-core-base.c:1823) i2c_core
  i2c_register_driver (drivers/i2c/i2c-core-base.c:1861) i2c_core
  do_one_initcall (init/main.c:1296)
  do_init_module (kernel/module/main.c:2455)
  ...
 </TASK>
 ---[ end trace 0000000000000000 ]---

Fix this problem by correctly set piix4_adapter_count as 1 for the
single adapter so it can be normally removed.

Fixes: 528d53a1592b ("i2c: piix4: Fix probing of reserved ports on AMD Family 16h Model 30h")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
v1 -> v2:
Set piix4_adapter_count as 1 rather than increase it and slightly fix
the commit message.
---
 drivers/i2c/busses/i2c-piix4.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jean Delvare Oct. 27, 2022, 2:12 p.m. UTC | #1 
On Thu, 27 Oct 2022 20:13:53 +0800, Chen Zhongjin wrote:
> In piix4_probe(), the piix4 adapter will be registered in:
> 
>    piix4_probe()
>      piix4_add_adapters_sb800() / piix4_add_adapter()
>        i2c_add_adapter()
> 
> Based on the probed device type, piix4_add_adapters_sb800() or single
> piix4_add_adapter() will be called.
> For the former case, piix4_adapter_count is set as the number of adapters,
> while for antoher case it is not set and kept default *zero*.
> 
> When piix4 is removed, piix4_remove() removes the adapters added in
> piix4_probe(), basing on the piix4_adapter_count value.
> Because the count is zero for the single adapter case, the adapter won't
> be removed and makes the sources allocated for adapter leaked, such as
> the i2c client and device.
> 
> These sources can still be accessed by i2c or bus and cause problems.
> An easily reproduced case is that if a new adapter is registered, i2c
> will get the leaked adapter and try to call smbus_algorithm, which was
> already freed:
> 
> Triggered by: rmmod i2c_piix4 && modprobe max31730
> 
>  BUG: unable to handle page fault for address: ffffffffc053d860
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  Oops: 0000 [#1] PREEMPT SMP KASAN
>  CPU: 0 PID: 3752 Comm: modprobe Tainted: G
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>  RIP: 0010:i2c_default_probe (drivers/i2c/i2c-core-base.c:2259) i2c_core
>  RSP: 0018:ffff888107477710 EFLAGS: 00000246
>  ...
>  <TASK>
>   i2c_detect (drivers/i2c/i2c-core-base.c:2302) i2c_core
>   __process_new_driver (drivers/i2c/i2c-core-base.c:1336) i2c_core
>   bus_for_each_dev (drivers/base/bus.c:301)
>   i2c_for_each_dev (drivers/i2c/i2c-core-base.c:1823) i2c_core
>   i2c_register_driver (drivers/i2c/i2c-core-base.c:1861) i2c_core
>   do_one_initcall (init/main.c:1296)
>   do_init_module (kernel/module/main.c:2455)
>   ...
>  </TASK>
>  ---[ end trace 0000000000000000 ]---
> 
> Fix this problem by correctly set piix4_adapter_count as 1 for the
> single adapter so it can be normally removed.
> 
> Fixes: 528d53a1592b ("i2c: piix4: Fix probing of reserved ports on AMD Family 16h Model 30h")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
> v1 -> v2:
> Set piix4_adapter_count as 1 rather than increase it and slightly fix
> the commit message.
> ---
>  drivers/i2c/busses/i2c-piix4.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 39cb1b7bb865..809fbd014cd6 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -1080,6 +1080,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  					   "", &piix4_main_adapters[0]);
>  		if (retval < 0)
>  			return retval;
> +		piix4_adapter_count = 1;
>  	}
>  
>  	/* Check for auxiliary SMBus on some AMD chipsets */

Reviewed-by: Jean Delvare <jdelvare@suse.de>

I believe this qualifies for stable trees, so:

Cc: stable@vger.kernel.org

Thanks,
Wolfram Sang Nov. 1, 2022, 12:29 p.m. UTC | #2 
On Thu, Oct 27, 2022 at 08:13:53PM +0800, Chen Zhongjin wrote:
> In piix4_probe(), the piix4 adapter will be registered in:
> 
>    piix4_probe()
>      piix4_add_adapters_sb800() / piix4_add_adapter()
>        i2c_add_adapter()
> 
> Based on the probed device type, piix4_add_adapters_sb800() or single
> piix4_add_adapter() will be called.
> For the former case, piix4_adapter_count is set as the number of adapters,
> while for antoher case it is not set and kept default *zero*.
> 
> When piix4 is removed, piix4_remove() removes the adapters added in
> piix4_probe(), basing on the piix4_adapter_count value.
> Because the count is zero for the single adapter case, the adapter won't
> be removed and makes the sources allocated for adapter leaked, such as
> the i2c client and device.
> 
> These sources can still be accessed by i2c or bus and cause problems.
> An easily reproduced case is that if a new adapter is registered, i2c
> will get the leaked adapter and try to call smbus_algorithm, which was
> already freed:
> 
> Triggered by: rmmod i2c_piix4 && modprobe max31730
> 
>  BUG: unable to handle page fault for address: ffffffffc053d860
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  Oops: 0000 [#1] PREEMPT SMP KASAN
>  CPU: 0 PID: 3752 Comm: modprobe Tainted: G
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>  RIP: 0010:i2c_default_probe (drivers/i2c/i2c-core-base.c:2259) i2c_core
>  RSP: 0018:ffff888107477710 EFLAGS: 00000246
>  ...
>  <TASK>
>   i2c_detect (drivers/i2c/i2c-core-base.c:2302) i2c_core
>   __process_new_driver (drivers/i2c/i2c-core-base.c:1336) i2c_core
>   bus_for_each_dev (drivers/base/bus.c:301)
>   i2c_for_each_dev (drivers/i2c/i2c-core-base.c:1823) i2c_core
>   i2c_register_driver (drivers/i2c/i2c-core-base.c:1861) i2c_core
>   do_one_initcall (init/main.c:1296)
>   do_init_module (kernel/module/main.c:2455)
>   ...
>  </TASK>

Applied to for-current, thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 39cb1b7bb865..809fbd014cd6 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -1080,6 +1080,7 @@  static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 					   "", &piix4_main_adapters[0]);
 		if (retval < 0)
 			return retval;
+		piix4_adapter_count = 1;
 	}
 
 	/* Check for auxiliary SMBus on some AMD chipsets */