Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks
       [not found] <CGME20200522101524eucas1p1aeef4a054a80b5d822ed3dc4b16139d7@eucas1p1.samsung.com>
@ 2020-05-22 10:13 ` Marek Szyprowski
  2020-05-22 11:15   ` Marek Szyprowski
  0 siblings, 1 reply; 5+ messages in thread
From: Marek Szyprowski @ 2020-05-22 10:13 UTC (permalink / raw)
  To: linux-pm, linux-i2c, linux-kernel
  Cc: Marek Szyprowski, Wolfram Sang, Bibby Hsieh, Tomasz Figa,
	Bartosz Golaszewski, srv_heupstream, Bartlomiej Zolnierkiewicz,
	linux-samsung-soc

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks
  2020-05-22 10:13 ` [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks Marek Szyprowski
@ 2020-05-22 11:15   ` Marek Szyprowski
  2020-05-22 14:20     ` Wolfram Sang
  2020-05-25 12:28     ` Tomasz Figa
  0 siblings, 2 replies; 5+ messages in thread
From: Marek Szyprowski @ 2020-05-22 11:15 UTC (permalink / raw)
  To: linux-pm, linux-i2c, linux-kernel
  Cc: Wolfram Sang, Bibby Hsieh, Tomasz Figa, Bartosz Golaszewski,
	srv_heupstream, Bartlomiej Zolnierkiewicz, linux-samsung-soc

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks
  2020-05-22 11:15   ` Marek Szyprowski
@ 2020-05-22 14:20     ` Wolfram Sang
  2020-05-25 12:28     ` Tomasz Figa
  1 sibling, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2020-05-22 14:20 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-pm, linux-i2c, linux-kernel, Bibby Hsieh, Tomasz Figa,
	Bartosz Golaszewski, srv_heupstream, Bartlomiej Zolnierkiewicz,
	linux-samsung-soc


[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks
  2020-05-22 11:15   ` Marek Szyprowski
  2020-05-22 14:20     ` Wolfram Sang
@ 2020-05-25 12:28     ` Tomasz Figa
  2020-05-25 12:43       ` Marek Szyprowski
  1 sibling, 1 reply; 5+ messages in thread
From: Tomasz Figa @ 2020-05-25 12:28 UTC (permalink / raw)
  To: Marek Szyprowski, Bibby Hsieh
  Cc: Linux PM, linux-i2c, Linux Kernel Mailing List, Wolfram Sang,
	Bartosz Golaszewski, srv_heupstream, Bartlomiej Zolnierkiewicz,
	linux-samsung-soc

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks
  2020-05-25 12:28     ` Tomasz Figa
@ 2020-05-25 12:43       ` Marek Szyprowski
  0 siblings, 0 replies; 5+ messages in thread
From: Marek Szyprowski @ 2020-05-25 12:43 UTC (permalink / raw)
  To: Tomasz Figa, Bibby Hsieh
  Cc: Linux PM, linux-i2c, Linux Kernel Mailing List, Wolfram Sang,
	Bartosz Golaszewski, srv_heupstream, Bartlomiej Zolnierkiewicz,
	linux-samsung-soc

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200522101524eucas1p1aeef4a054a80b5d822ed3dc4b16139d7@eucas1p1.samsung.com>
2020-05-22 10:13 ` [PATCH] i2c: core: fix NULL pointer dereference in suspend/resume callbacks Marek Szyprowski
2020-05-22 11:15   ` Marek Szyprowski
2020-05-22 14:20     ` Wolfram Sang
2020-05-25 12:28     ` Tomasz Figa
2020-05-25 12:43       ` Marek Szyprowski

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git