[PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Peter Huewe]

If adapter->dev.parent == NULL there is a NULL pointer dereference in
acpi_i2c_install_space_handler and acpi_i2c_remove_space_handler.

This is present since introduction of this code:
366047515c6e "i2c: rework kernel config I2C_ACPI" or even
da3c6647ee08 "I2C/ACPI: Clean up I2C ACPI code and Add CONFIG_I2C_ACPI"

The adapter->dev.parent == NULL case is valid for the i2c_stub,
so loading i2c_stub with ACPI_I2C_OPREGION enabled results in an oops.
This is also valid at least for i2c_tiny_usb and i2c_robotfuzz_osif.

Fix by checking whether it is null before calling ACPI_HANDLE.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/i2c/i2c-acpi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
index 0dbc18c..c456b17 100644
--- a/drivers/i2c/i2c-acpi.c
+++ b/drivers/i2c/i2c-acpi.c
@@ -308,10 +308,15 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 
 int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
 {
-	acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+	acpi_handle handle;
 	struct acpi_i2c_handler_data *data;
 	acpi_status status;
 
+	if (!adapter->dev.parent)
+		return -ENODEV;
+
+	handle = ACPI_HANDLE(adapter->dev.parent);
+
 	if (!handle)
 		return -ENODEV;
 
@@ -344,10 +349,15 @@ int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
 
 void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
 {
-	acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+	acpi_handle handle;
 	struct acpi_i2c_handler_data *data;
 	acpi_status status;
 
+	if (!adapter->dev.parent)
+		return;
+
+	handle = ACPI_HANDLE(adapter->dev.parent);
+
 	if (!handle)
 		return;
 
-- 
1.8.5.5

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Peter Hüwe]

Am Freitag, 12. September 2014, 21:09:47 schrieb Peter Huewe:
> If adapter->dev.parent == NULL there is a NULL pointer dereference in
> acpi_i2c_install_space_handler and acpi_i2c_remove_space_handler.
> 
> This is present since introduction of this code:
> 366047515c6e "i2c: rework kernel config I2C_ACPI" or even
> da3c6647ee08 "I2C/ACPI: Clean up I2C ACPI code and Add CONFIG_I2C_ACPI"
> 
> The adapter->dev.parent == NULL case is valid for the i2c_stub,
> so loading i2c_stub with ACPI_I2C_OPREGION enabled results in an oops.
> This is also valid at least for i2c_tiny_usb and i2c_robotfuzz_osif.
> 
> Fix by checking whether it is null before calling ACPI_HANDLE.
> 
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> ---

Patch against current i2c/master.

For those who care - here's the oops:
# modprobe i2c_stub chip_addr=0x20
# dmesg

[   39.315090] i2c-stub: Virtual chip at 0x20
[   39.315149] BUG: unable to handle kernel NULL pointer dereference at 
0000000000000240
[   39.317716] IP: [<ffffffff8248ed65>] acpi_i2c_install_space_handler+0x16/0xb2
[   39.320261] PGD 40db4b067 PUD 40d2bf067 PMD 0 
[   39.322848] Oops: 0000 [#1] PREEMPT SMP 
[   39.325360] Modules linked in: i2c_stub(+) w83627ehf hwmon_vid ipv6 usbhid 
snd_hda_codec_hdmi x86_pkg_temp_thermal snd_hda_codec_realtek coretemp 
snd_hda_codec_generic kvm_intel kvm crc32_pclmul ghash_clmulni_intel snd_hda_intel 
snd_hda_controller pcspkr snd_hda_codec i2c_i801 snd_hwdep snd_pcm snd_timer snd 
battery tpm_tis tpm
[   39.330770] CPU: 0 PID: 2783 Comm: modprobe Not tainted 3.17.0-rc4-00131-gd030671 
#151
[   39.333451] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z77 Pro4, 
BIOS P1.70 01/17/2013
[   39.336153] task: ffff88040e4bd7d0 ti: ffff88040e60c000 task.ti: ffff88040e60c000
[   39.338876] RIP: 0010:[<ffffffff8248ed65>]  [<ffffffff8248ed65>] 
acpi_i2c_install_space_handler+0x16/0xb2
[   39.341657] RSP: 0018:ffff88040e60fca8  EFLAGS: 00010296
[   39.344421] RAX: 0000000000000000 RBX: ffffffffc099db30 RCX: ffff88040d8def40
[   39.347193] RDX: 00000000ffffffed RSI: ffff8800bff975e0 RDI: ffffffffc099db30
[   39.349965] RBP: ffff88040e60fcc8 R08: ffff8800bff975e0 R09: ffff8800bff975e0
[   39.352742] R10: ffffffffc099db78 R11: ffff88040b51c028 R12: ffffffffc099db78
[   39.355510] R13: ffffffffc099db30 R14: 0000000000000000 R15: ffffffffc099ded0
[   39.358275] FS:  00007f638fd52700(0000) GS:ffff88041f200000(0000) 
knlGS:0000000000000000
[   39.361008] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   39.363681] CR2: 0000000000000240 CR3: 000000040dbba000 CR4: 00000000001407f0
[   39.366332] Stack:
[   39.368924]  ffff88040d8def40 ffffffffc099db30 ffffffffc099db78 0000000000000000
[   39.371576]  ffff88040e60fcf8 ffffffff8248e2ee ffffffffc099db30 0000000000000001
[   39.374216]  0000000000000000 ffffffff82782250 ffff88040e60fd28 ffffffff8248e424
[   39.376840] Call Trace:
[   39.379424]  [<ffffffff8248e2ee>] i2c_register_adapter+0x1bc/0x299
[   39.382044]  [<ffffffff8248e424>] i2c_add_adapter+0x59/0x60
[   39.384650]  [<ffffffffc09a01b6>] i2c_stub_init+0x1b6/0x1d4 [i2c_stub]
[   39.387277]  [<ffffffffc09a0000>] ? 0xffffffffc09a0000
[   39.389896]  [<ffffffffc09a0000>] ? 0xffffffffc09a0000
[   39.392504]  [<ffffffff8200030e>] do_one_initcall+0xea/0x184
[   39.395128]  [<ffffffff82172a63>] ? vfree+0x74/0x7b
[   39.397763]  [<ffffffff82109550>] load_module+0x1b0f/0x1e11
[   39.397768]  [<ffffffff82106d13>] ? module_unload_free+0xd2/0xd2
[   39.397773]  [<ffffffff82109943>] SyS_finit_module+0x56/0x6c
[   39.397779]  [<ffffffff8255fdcb>] tracesys+0xdd/0xe2
[   39.397822] Code: 48 c7 c6 37 37 70 82 31 c0 e8 56 66 f5 ff 48 83 c4 18 5b 5d c3 
55 ba ed ff ff ff 48 89 e5 41 55 49 89 fd 41 54 53 51 48 8b 47 48 <48> 8b 80 40 02 00 
00 48 85 c0 0f 84 82 00 00 00 4c 8b 60 08 4d 
[   39.397827] RIP  [<ffffffff8248ed65>] acpi_i2c_install_space_handler+0x16/0xb2
[   39.397828]  RSP <ffff88040e60fca8>
[   39.397829] CR2: 0000000000000240
[   39.397863] ---[ end trace 9f55e6ce67aaaafb ]---

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Mika Westerberg]

On Fri, Sep 12, 2014 at 09:09:47PM +0200, Peter Huewe wrote:
> If adapter->dev.parent == NULL there is a NULL pointer dereference in
> acpi_i2c_install_space_handler and acpi_i2c_remove_space_handler.
> 
> This is present since introduction of this code:
> 366047515c6e "i2c: rework kernel config I2C_ACPI" or even
> da3c6647ee08 "I2C/ACPI: Clean up I2C ACPI code and Add CONFIG_I2C_ACPI"
> 
> The adapter->dev.parent == NULL case is valid for the i2c_stub,
> so loading i2c_stub with ACPI_I2C_OPREGION enabled results in an oops.
> This is also valid at least for i2c_tiny_usb and i2c_robotfuzz_osif.
> 
> Fix by checking whether it is null before calling ACPI_HANDLE.
> 
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Wolfram Sang]

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

On Wed, Sep 24, 2014 at 11:06:24PM +0200, Rafael J. Wysocki wrote:
> Hi Wolfram,
> 
> Do you want me to take care of this or are you going to handle it?
> 
> It has been ACKed by Mika already: https://patchwork.kernel.org/patch/4897751/

I am going to handle it. i2c-acpi is being reworked, currently. Just
waiting for some testbot results.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Rafael J. Wysocki]

Hi Wolfram,

Do you want me to take care of this or are you going to handle it?

It has been ACKed by Mika already: https://patchwork.kernel.org/patch/4897751/

Rafael


On Friday, September 12, 2014 09:09:47 PM Peter Huewe wrote:
> If adapter->dev.parent == NULL there is a NULL pointer dereference in
> acpi_i2c_install_space_handler and acpi_i2c_remove_space_handler.
> 
> This is present since introduction of this code:
> 366047515c6e "i2c: rework kernel config I2C_ACPI" or even
> da3c6647ee08 "I2C/ACPI: Clean up I2C ACPI code and Add CONFIG_I2C_ACPI"
> 
> The adapter->dev.parent == NULL case is valid for the i2c_stub,
> so loading i2c_stub with ACPI_I2C_OPREGION enabled results in an oops.
> This is also valid at least for i2c_tiny_usb and i2c_robotfuzz_osif.
> 
> Fix by checking whether it is null before calling ACPI_HANDLE.
> 
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> ---
>  drivers/i2c/i2c-acpi.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
> index 0dbc18c..c456b17 100644
> --- a/drivers/i2c/i2c-acpi.c
> +++ b/drivers/i2c/i2c-acpi.c
> @@ -308,10 +308,15 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
>  
>  int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
>  {
> -	acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
> +	acpi_handle handle;
>  	struct acpi_i2c_handler_data *data;
>  	acpi_status status;
>  
> +	if (!adapter->dev.parent)
> +		return -ENODEV;
> +
> +	handle = ACPI_HANDLE(adapter->dev.parent);
> +
>  	if (!handle)
>  		return -ENODEV;
>  
> @@ -344,10 +349,15 @@ int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
>  
>  void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
>  {
> -	acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
> +	acpi_handle handle;
>  	struct acpi_i2c_handler_data *data;
>  	acpi_status status;
>  
> +	if (!adapter->dev.parent)
> +		return;
> +
> +	handle = ACPI_HANDLE(adapter->dev.parent);
> +
>  	if (!handle)
>  		return;
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Wolfram Sang]

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


> I'm dropping it from my TODO list, then.

Yup. Thanks for the heads up!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Rafael J. Wysocki]

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

On Wednesday, September 24, 2014 11:02:16 PM Wolfram Sang wrote:
> On Wed, Sep 24, 2014 at 11:06:24PM +0200, Rafael J. Wysocki wrote:
> > Hi Wolfram,
> > 
> > Do you want me to take care of this or are you going to handle it?
> > 
> > It has been ACKed by Mika already: https://patchwork.kernel.org/patch/4897751/
> 
> I am going to handle it. i2c-acpi is being reworked, currently. Just
> waiting for some testbot results.

Cool, thanks!

I'm dropping it from my TODO list, then.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] [REGRESSION] i2c-acpi: Fix NULL Pointer dereference

[Wolfram Sang]

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

On Fri, Sep 12, 2014 at 09:09:47PM +0200, Peter Huewe wrote:
> If adapter->dev.parent == NULL there is a NULL pointer dereference in
> acpi_i2c_install_space_handler and acpi_i2c_remove_space_handler.
> 
> This is present since introduction of this code:
> 366047515c6e "i2c: rework kernel config I2C_ACPI" or even
> da3c6647ee08 "I2C/ACPI: Clean up I2C ACPI code and Add CONFIG_I2C_ACPI"
> 
> The adapter->dev.parent == NULL case is valid for the i2c_stub,
> so loading i2c_stub with ACPI_I2C_OPREGION enabled results in an oops.
> This is also valid at least for i2c_tiny_usb and i2c_robotfuzz_osif.
> 
> Fix by checking whether it is null before calling ACPI_HANDLE.
> 
> Signed-off-by: Peter Huewe <peterhuewe-Mmb7MZpHnFY@public.gmane.org>

Applied to for-current, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]