[PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks

[PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks

[Marek Szyprowski]

Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in
adapter") added generic suspend and resume functions for i2c devices.
Those functions unconditionally access an i2c_client structure assigned
to the given i2c device. However, there exist i2c devices in the system
without a valid i2c_client. Add the needed check before accessing the
i2c_client.

This fixes the following issue observed on Samsung Exynos4412-based
Odroid U3 board:

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000018
pgd = 2aed198a
[00000018] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 1295 Comm: rtcwake Not tainted 5.7.0-rc6-02700-g4773d1324da6 #739
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at i2c_suspend_late+0x20/0x48
LR is at dpm_run_callback+0xb4/0x3fc
pc : [<c07b404c>]    lr : [<c064b7cc>]    psr: 20000053
...
Process rtcwake (pid: 1295, stack limit = 0x7f1885cf)
Stack: (0xec8f3d70 to 0xec8f4000)
...
[<c07b404c>] (i2c_suspend_late) from [<c064b7cc>] (dpm_run_callback+0xb4/0x3fc)
[<c064b7cc>] (dpm_run_callback) from [<c064ce04>] (__device_suspend_late+0xcc/0x16c)
[<c064ce04>] (__device_suspend_late) from [<c064f0b0>] (dpm_suspend_late+0x10c/0x568)
[<c064f0b0>] (dpm_suspend_late) from [<c01996f0>] (suspend_devices_and_enter+0x31c/0xc70)
[<c01996f0>] (suspend_devices_and_enter) from [<c019a43c>] (pm_suspend+0x3f8/0x480)
[<c019a43c>] (pm_suspend) from [<c0198174>] (state_store+0x6c/0xc8)
[<c0198174>] (state_store) from [<c035cf4c>] (kernfs_fop_write+0x10c/0x228)
[<c035cf4c>] (kernfs_fop_write) from [<c02b94a4>] (__vfs_write+0x30/0x1d0)
[<c02b94a4>] (__vfs_write) from [<c02bc444>] (vfs_write+0xa4/0x170)
[<c02bc444>] (vfs_write) from [<c02bc690>] (ksys_write+0x60/0xd8)
[<c02bc690>] (ksys_write) from [<c0100060>] (ret_fast_syscall+0x0/0x28)
Exception stack(0xec8f3fa8 to 0xec8f3ff0)
...
---[ end trace a43afef431782f37 ]---

Fixes: 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in adapter")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
This fixes suspend/resume issue observed on various board with linux-next
from 20200521.
---
 drivers/i2c/i2c-core-base.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5be24bf8a194..b531f5ad06b2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -454,11 +454,13 @@ static int i2c_device_remove(struct device *dev)
 static int i2c_resume_early(struct device *dev)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_adapter *adap = client->adapter;
 	int err;
 
+	if (!client)
+		return 0;
+
 	if (!pm_runtime_status_suspended(&client->dev)) {
-		err = regulator_enable(adap->bus_regulator);
+		err = regulator_enable(client->adapter->bus_regulator);
 		if (err)
 			return err;
 	}
@@ -469,15 +471,17 @@ static int i2c_resume_early(struct device *dev)
 static int i2c_suspend_late(struct device *dev)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_adapter *adap = client->adapter;
 	int err;
 
+	if (!client)
+		return 0;
+
 	err = pm_generic_suspend_late(&client->dev);
 	if (err)
 		return err;
 
 	if (!pm_runtime_status_suspended(&client->dev))
-		return regulator_disable(adap->bus_regulator);
+		return regulator_disable(client->adapter->bus_regulator);
 
 	return 0;
 }
@@ -487,10 +491,12 @@ static int i2c_suspend_late(struct device *dev)
 static int i2c_runtime_resume(struct device *dev)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_adapter *adap = client->adapter;
 	int err;
 
-	err = regulator_enable(adap->bus_regulator);
+	if (!client)
+		return 0;
+
+	err = regulator_enable(client->adapter->bus_regulator);
 	if (err)
 		return err;
 
@@ -500,14 +506,16 @@ static int i2c_runtime_resume(struct device *dev)
 static int i2c_runtime_suspend(struct device *dev)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_adapter *adap = client->adapter;
 	int err;
 
+	if (!client)
+		return 0;
+
 	err = pm_generic_runtime_suspend(&client->dev);
 	if (err)
 		return err;
 
-	return regulator_disable(adap->bus_regulator);
+	return regulator_disable(client->adapter->bus_regulator);
 }
 #endif
 
-- 
2.17.1

Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks

[Marek Szyprowski]

Hi All,

On 22.05.2020 12:13, Marek Szyprowski wrote:
> Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in
> adapter") added generic suspend and resume functions for i2c devices.
> Those functions unconditionally access an i2c_client structure assigned
> to the given i2c device. However, there exist i2c devices in the system
> without a valid i2c_client. Add the needed check before accessing the
> i2c_client.

Just one more comment. The devices without i2c_client structure are the 
i2c 'devices' associated with the respective i2c bus. They are visible 
in /sys:

ls -l /sys/bus/i2c/devices/i2c-*

I wonder if this patch has been ever tested with system suspend/resume, 
as those devices are always available in the system...

> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Fri, May 22, 2020 at 01:15:12PM +0200, Marek Szyprowski wrote:
> Hi All,
> 
> On 22.05.2020 12:13, Marek Szyprowski wrote:
> > Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in
> > adapter") added generic suspend and resume functions for i2c devices.
> > Those functions unconditionally access an i2c_client structure assigned
> > to the given i2c device. However, there exist i2c devices in the system
> > without a valid i2c_client. Add the needed check before accessing the
> > i2c_client.
> 
> Just one more comment. The devices without i2c_client structure are the 
> i2c 'devices' associated with the respective i2c bus. They are visible 
> in /sys:
> 
> ls -l /sys/bus/i2c/devices/i2c-*
> 
> I wonder if this patch has been ever tested with system suspend/resume, 
> as those devices are always available in the system...

There was another issue with this patch. Although it is not clear yet,
if the patch itself is the culprit or if it just unshadows something
else, however, I am considering to just revert it until these issues are
fixed.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks

[Tomasz Figa]

Hi Marek,

On Fri, May 22, 2020 at 1:15 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi All,
>
> On 22.05.2020 12:13, Marek Szyprowski wrote:
> > Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in
> > adapter") added generic suspend and resume functions for i2c devices.
> > Those functions unconditionally access an i2c_client structure assigned
> > to the given i2c device. However, there exist i2c devices in the system
> > without a valid i2c_client. Add the needed check before accessing the
> > i2c_client.
>
> Just one more comment. The devices without i2c_client structure are the
> i2c 'devices' associated with the respective i2c bus. They are visible
> in /sys:
>
> ls -l /sys/bus/i2c/devices/i2c-*
>
> I wonder if this patch has been ever tested with system suspend/resume,
> as those devices are always available in the system...

Sorry for the trouble and thanks a lot for the fix. We'll make sure to
do more thorough testing, including suspend/resume before relanding
this change.

Since the patch was reverted, can we squash your fix with the next
revision together with your Co-developed-by and Signed-off-by tags?

Best regards,
Tomasz

Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks

[Marek Szyprowski]

Hi Tomasz

On 25.05.2020 14:28, Tomasz Figa wrote:
> On Fri, May 22, 2020 at 1:15 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 22.05.2020 12:13, Marek Szyprowski wrote:
>>> Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in
>>> adapter") added generic suspend and resume functions for i2c devices.
>>> Those functions unconditionally access an i2c_client structure assigned
>>> to the given i2c device. However, there exist i2c devices in the system
>>> without a valid i2c_client. Add the needed check before accessing the
>>> i2c_client.
>> Just one more comment. The devices without i2c_client structure are the
>> i2c 'devices' associated with the respective i2c bus. They are visible
>> in /sys:
>>
>> ls -l /sys/bus/i2c/devices/i2c-*
>>
>> I wonder if this patch has been ever tested with system suspend/resume,
>> as those devices are always available in the system...
> Sorry for the trouble and thanks a lot for the fix. We'll make sure to
> do more thorough testing, including suspend/resume before relanding
> this change.
>
> Since the patch was reverted, can we squash your fix with the next
> revision together with your Co-developed-by and Signed-off-by tags?
Sure, no problem. The fix is trivial.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland