Message ID | 20221025024456.110090-1-chenzhongjin@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp774786wru; Mon, 24 Oct 2022 20:05:34 -0700 (PDT) X-Google-Smtp-Source: AMsMyM72ZN7wjioxGhoiMccef4hfjVbcYhQRpWKqEKrSs6c+e1CQsKQMWyPDtbhSp5zcY0XA/SN3 X-Received: by 2002:a05:6402:50d3:b0:461:ba8a:8779 with SMTP id h19-20020a05640250d300b00461ba8a8779mr8636260edb.411.1666667134775; Mon, 24 Oct 2022 20:05:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666667134; cv=none; d=google.com; s=arc-20160816; b=hbElXUS1Q5c/wXz+HvEjs0bIGJClBVcEcseli+Urb1UFNoOht/v8Kp+LTO6wEdvr37 WjlyaA/hC+7u/Gp6KMhi/2IjGI7d3a5Rm6umBtMu6KIOgBaJJ3zb0933t/ada851FgWK aezgW/cOTHlEr5zuY/Yn0hqanzwx19y3U4WHJ5H0cgKynSv4KBIb+fK3dhFUfje3wiCY 1rDstOEUeCDMJHaytqXg8JRN4gpkYLub4kWX6Z1seHf5GLcLC+fLAOCRyDgVx/gjXhzT MNZMZtWYA/slz1Qjnh7CZ1McmIxJuIUuujgYPNE9a3Syq8tJGrnwNwT+v4iZ9Anq4OBv af3g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:subject:cc:to:from; bh=Jy8IpiRshnJhSo78oV7HBn5volyXOr5BcyKDqEUFiFQ=; b=pAYK5g4uYRHxE7Ygb6B0JMD1cjNJsuIPM84svgPyRpcqujcedyXLXzz+shHg/2HUag XplAZqxja2tfw9d5VjdJod+9495R0btJxf2GGf9LXx1j/sDjzFyx6GLWOg+iEX109Et/ ws4M+8hCk07rLkkeFvAMAuPsGbiK9sfGKVA/jbNAHOBbw73UnZ63RPGBexMjrQN74R9x n59YhT6N4wBRTeG2R6KqhJ09CSQjLcj9z/tc6MF3Z92rR818zJguDdVRDjsOSSqQDjpo Bj5z9qXx8+oqqNUE2GFRQDK7aTK+IJdRtEwNzJLKQNzd1Rw4bSwBrH9ZaN9yjR7rwW7n BU7A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id mm26-20020a170906cc5a00b007ab34aa90acsi1146596ejb.461.2022.10.24.20.05.05; Mon, 24 Oct 2022 20:05:34 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230179AbiJYCsX (ORCPT <rfc822;pwkd43@gmail.com> + 99 others); Mon, 24 Oct 2022 22:48:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231537AbiJYCsV (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 24 Oct 2022 22:48:21 -0400 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 35689FF225; Mon, 24 Oct 2022 19:48:20 -0700 (PDT) Received: from dggpemm500021.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4MxGTd0f3PzVj0Z; Tue, 25 Oct 2022 10:43:33 +0800 (CST) Received: from dggpemm500013.china.huawei.com (7.185.36.172) by dggpemm500021.china.huawei.com (7.185.36.109) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 25 Oct 2022 10:48:18 +0800 Received: from ubuntu1804.huawei.com (10.67.175.36) by dggpemm500013.china.huawei.com (7.185.36.172) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 25 Oct 2022 10:48:18 +0800 From: Chen Zhongjin <chenzhongjin@huawei.com> To: <linux-kernel@vger.kernel.org>, <linux-i2c@vger.kernel.org> CC: <jdelvare@suse.com>, <wsa@kernel.org>, <chenzhongjin@huawei.com> Subject: [PATCH] i2c: piix4: Fix adapter not be removed in piix4_remove() Date: Tue, 25 Oct 2022 10:44:56 +0800 Message-ID: <20221025024456.110090-1-chenzhongjin@huawei.com> X-Mailer: git-send-email 2.17.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.67.175.36] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggpemm500013.china.huawei.com (7.185.36.172) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747627157473460257?= X-GMAIL-MSGID: =?utf-8?q?1747627157473460257?= |
Series |
i2c: piix4: Fix adapter not be removed in piix4_remove()
|
|
Commit Message
Chen Zhongjin
Oct. 25, 2022, 2:44 a.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 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(+)
Comments
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,
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 */