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

  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 for the single
adapter path so the adapter can be normally removed in piix4_remove().

Fixes: 528d53a1592b ("i2c: piix4: Fix probing of reserved ports on AMD Family 16h Model 30h")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 drivers/i2c/busses/i2c-piix4.c | 1 +
 1 file changed, 1 insertion(+)
  

Hi Chen,

On Tue, 25 Oct 2022 10:44:56 +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 for the single
> adapter path so the adapter can be normally removed in piix4_remove().
> 
> Fixes: 528d53a1592b ("i2c: piix4: Fix probing of reserved ports on AMD Family 16h Model 30h")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>

Nice catch, and sorry for introducing this bug in the first place.

I'm not fully happy with your fix though.

> ---
>  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..125646fd36dc 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++;
>  	}
>  
>  	/* Check for auxiliary SMBus on some AMD chipsets */

Fundamentally, you want to set piix4_adapter_count to 1. You use ++
based on the assumption that piix4_adapter_count is 0 initially. While
this is true upon loading the driver, it would no longer be true is an
adapter has already been registered and then unregistered without
unloading the driver. This could happen if the SMBus controller is
hot-plugged/unplugged (I am not aware of this happening in the real
world, to be honest) or if the system owner manually unbinds then
rebinds the device to the i2c-piix4 driver (something a kernel
developer could legitimately do to exercise or otherwise test the
probing and removal code paths of the driver).

So I think that the following sequence would cause piix4_adapter_count
to grow beyond PIIX4_MAX_ADAPTERS with your patch applied (adjust the
PCI device bus location according to your system), which in turn would
result in an array overrun in piix4_remove():

# for n in `seq 1 8` ; do echo "0000:00:14.0" > /sys/bus/pci/drivers/piix4_smbus/unbind ; echo "0000:00:14.0" > /sys/bus/pci/drivers/piix4_smbus/bind ; done

For this reason, I am asking that you explicitly set
piix4_adapter_count to 1 instead of incrementing it.

Thanks,
  

Hi Jean,

On 2022/10/27 19:10, Jean Delvare wrote:
> Hi Chen,
>
> On Tue, 25 Oct 2022 10:44:56 +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 for the single
>> adapter path so the adapter can be normally removed in piix4_remove().
>>
>> Fixes: 528d53a1592b ("i2c: piix4: Fix probing of reserved ports on AMD Family 16h Model 30h")
>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> Nice catch, and sorry for introducing this bug in the first place.
>
> I'm not fully happy with your fix though.
>
>> ---
>>   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..125646fd36dc 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++;
>>   	}
>>   
>>   	/* Check for auxiliary SMBus on some AMD chipsets */
> Fundamentally, you want to set piix4_adapter_count to 1. You use ++
> based on the assumption that piix4_adapter_count is 0 initially. While
> this is true upon loading the driver, it would no longer be true is an
> adapter has already been registered and then unregistered without
> unloading the driver. This could happen if the SMBus controller is
> hot-plugged/unplugged (I am not aware of this happening in the real
> world, to be honest) or if the system owner manually unbinds then
> rebinds the device to the i2c-piix4 driver (something a kernel
> developer could legitimately do to exercise or otherwise test the
> probing and removal code paths of the driver).

Thanks for your review and advice!

You are right that piix4_adapter_count should be set to 1.

Had sent v2 to fix it.


Best,

Chen

> So I think that the following sequence would cause piix4_adapter_count
> to grow beyond PIIX4_MAX_ADAPTERS with your patch applied (adjust the
> PCI device bus location according to your system), which in turn would
> result in an array overrun in piix4_remove():
>
> # for n in `seq 1 8` ; do echo "0000:00:14.0" > /sys/bus/pci/drivers/piix4_smbus/unbind ; echo "0000:00:14.0" > /sys/bus/pci/drivers/piix4_smbus/bind ; done
>
> For this reason, I am asking that you explicitly set
> piix4_adapter_count to 1 instead of incrementing it.
>
> Thanks,