[PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Xiubo Li]

Since we cannot make sure the 'data_len' will always be none zero here,
and then if 'data_len' equals to zero, the kzalloc() will return ZERO_SIZE_PTR,
which equals to ((void *)16).

So this patch fix this with just doing the 'data_len' zero check before calling
kzalloc().

Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/i2c/i2c-acpi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
index e8b6196..e144c00 100644
--- a/drivers/i2c/i2c-acpi.c
+++ b/drivers/i2c/i2c-acpi.c
@@ -134,6 +134,9 @@ static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
 	int ret;
 	u8 *buffer;
 
+	if (!data_len)
+		return -EINVAL;
+
 	buffer = kzalloc(data_len, GFP_KERNEL);
 	if (!buffer)
 		return AE_NO_MEMORY;
-- 
1.8.5

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Wolfram Sang]

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

On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> Since we cannot make sure the 'data_len' will always be none zero here,
> and then if 'data_len' equals to zero, the kzalloc() will return ZERO_SIZE_PTR,
> which equals to ((void *)16).

I assume the read request with length == 0 comes from a broken BIOS?

> So this patch fix this with just doing the 'data_len' zero check before calling
> kzalloc().
> 
> Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Looks good to me, yet adding ACPI experts to CC for further comments.

> ---
>  drivers/i2c/i2c-acpi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
> index e8b6196..e144c00 100644
> --- a/drivers/i2c/i2c-acpi.c
> +++ b/drivers/i2c/i2c-acpi.c
> @@ -134,6 +134,9 @@ static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
>  	int ret;
>  	u8 *buffer;
>  
> +	if (!data_len)
> +		return -EINVAL;
> +
>  	buffer = kzalloc(data_len, GFP_KERNEL);
>  	if (!buffer)
>  		return AE_NO_MEMORY;
> -- 
> 1.8.5
> 

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

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Mika Westerberg]

On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
> On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> > Since we cannot make sure the 'data_len' will always be none zero here,
> > and then if 'data_len' equals to zero, the kzalloc() will return ZERO_SIZE_PTR,
> > which equals to ((void *)16).
> 
> I assume the read request with length == 0 comes from a broken BIOS?

I'm also interested. Does this trigger in a real system?

> 
> > So this patch fix this with just doing the 'data_len' zero check before calling
> > kzalloc().
> > 
> > Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> 
> Looks good to me, yet adding ACPI experts to CC for further comments.
> 
> > ---
> >  drivers/i2c/i2c-acpi.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
> > index e8b6196..e144c00 100644
> > --- a/drivers/i2c/i2c-acpi.c
> > +++ b/drivers/i2c/i2c-acpi.c
> > @@ -134,6 +134,9 @@ static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
> >  	int ret;
> >  	u8 *buffer;
> >  
> > +	if (!data_len)
> > +		return -EINVAL;
> > +
> >  	buffer = kzalloc(data_len, GFP_KERNEL);
> >  	if (!buffer)
> >  		return AE_NO_MEMORY;
> > -- 
> > 1.8.5
> > 

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Wolfram Sang]

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

On Tue, Aug 19, 2014 at 06:16:49PM +0300, Mika Westerberg wrote:
> On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
> > On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> > > Since we cannot make sure the 'data_len' will always be none zero here,
> > > and then if 'data_len' equals to zero, the kzalloc() will return ZERO_SIZE_PTR,
> > > which equals to ((void *)16).
> > 
> > I assume the read request with length == 0 comes from a broken BIOS?
> 
> I'm also interested. Does this trigger in a real system?

Even if not now, we should consider potentially broken BIOSes, or? Which
extends the question to: Do we need even more sanity checks when taking
broken BIOSes into account?


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

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Mika Westerberg]

On Tue, Aug 19, 2014 at 10:38:08AM -0500, Wolfram Sang wrote:
> On Tue, Aug 19, 2014 at 06:16:49PM +0300, Mika Westerberg wrote:
> > On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
> > > On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> > > > Since we cannot make sure the 'data_len' will always be none zero here,
> > > > and then if 'data_len' equals to zero, the kzalloc() will return ZERO_SIZE_PTR,
> > > > which equals to ((void *)16).
> > > 
> > > I assume the read request with length == 0 comes from a broken BIOS?
> > 
> > I'm also interested. Does this trigger in a real system?
> 
> Even if not now, we should consider potentially broken BIOSes, or? Which
> extends the question to: Do we need even more sanity checks when taking
> broken BIOSes into account?

Typically ACPICA has done this work for us (e.g it fixes things upfront
so that we get sane data). I'm not sure if it does that for I2C
Operation Regions, though (that's why I'm asking if it happens in a real
system or is this more like a theoretical possibility).

Tianyu, any comments?

RE: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> On Tue, Aug 19, 2014 at 10:38:08AM -0500, Wolfram Sang wrote:
> > On Tue, Aug 19, 2014 at 06:16:49PM +0300, Mika Westerberg wrote:
> > > On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
> > > > On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> > > > > Since we cannot make sure the 'data_len' will always be none zero here,
> > > > > and then if 'data_len' equals to zero, the kzalloc() will return
> ZERO_SIZE_PTR,
> > > > > which equals to ((void *)16).
> > > >
> > > > I assume the read request with length == 0 comes from a broken BIOS?
> > >
> > > I'm also interested. Does this trigger in a real system?
> >
> > Even if not now, we should consider potentially broken BIOSes, or? Which
> > extends the question to: Do we need even more sanity checks when taking
> > broken BIOSes into account?
> 
> Typically ACPICA has done this work for us (e.g it fixes things upfront
> so that we get sane data). I'm not sure if it does that for I2C
> Operation Regions, though (that's why I'm asking if it happens in a real
> system or is this more like a theoretical possibility).
> 
Yes, it a theoretical potentially risk here for me, but not sure in the future.
I have encountered many issues of this risk for different codes, which can be
reproduced very easily mostly.

Thanks,

BRs
Xiubo

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Mika Westerberg]

On Wed, Aug 20, 2014 at 02:37:57AM +0000, Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org wrote:
> > On Tue, Aug 19, 2014 at 10:38:08AM -0500, Wolfram Sang wrote:
> > > On Tue, Aug 19, 2014 at 06:16:49PM +0300, Mika Westerberg wrote:
> > > > On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
> > > > > On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> > > > > > Since we cannot make sure the 'data_len' will always be none zero here,
> > > > > > and then if 'data_len' equals to zero, the kzalloc() will return
> > ZERO_SIZE_PTR,
> > > > > > which equals to ((void *)16).
> > > > >
> > > > > I assume the read request with length == 0 comes from a broken BIOS?
> > > >
> > > > I'm also interested. Does this trigger in a real system?
> > >
> > > Even if not now, we should consider potentially broken BIOSes, or? Which
> > > extends the question to: Do we need even more sanity checks when taking
> > > broken BIOSes into account?
> > 
> > Typically ACPICA has done this work for us (e.g it fixes things upfront
> > so that we get sane data). I'm not sure if it does that for I2C
> > Operation Regions, though (that's why I'm asking if it happens in a real
> > system or is this more like a theoretical possibility).
> > 
> Yes, it a theoretical potentially risk here for me, but not sure in the future.
> I have encountered many issues of this risk for different codes, which can be
> reproduced very easily mostly.

OK, so it didn̈́'t happen (yet :-)) in a real system.

I would like to check with Tianyu or Lv (Cc'd) whether ACPICA protects
us from this kind of insanity and if it doesn't right now, it probably
should.

I'm fine with the patch itself to be merged.

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Lan Tianyu]

On 08/19/2014 11:48 PM, Mika Westerberg wrote:
> On Tue, Aug 19, 2014 at 10:38:08AM -0500, Wolfram Sang wrote:
>> On Tue, Aug 19, 2014 at 06:16:49PM +0300, Mika Westerberg wrote:
>>> On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
>>>> On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
>>>>> Since we cannot make sure the 'data_len' will always be none zero
>>>>> here, and then if 'data_len' equals to zero, the kzalloc() will
>>>>> return ZERO_SIZE_PTR, which equals to ((void *)16).
>>>>
>>>> I assume the read request with length == 0 comes from a broken BIOS?
>>>
>>> I'm also interested. Does this trigger in a real system?
>>
>> Even if not now, we should consider potentially broken BIOSes, or? Which
>> extends the question to: Do we need even more sanity checks when taking
>> broken BIOSes into account?
>
> Typically ACPICA has done this work for us (e.g it fixes things upfront so
> that we get sane data). I'm not sure if it does that for I2C Operation
> Regions, though (that's why I'm asking if it happens in a real system or is
> this more like a theoretical possibility).
>
> Tianyu, any comments?
>

Sorry for later response due to leave home today. acpi_gsb_i2c_read_bytes()
dedicates for GenericSerialBus Read/Write N Bytes protocol(ACPI Spec
5.5.2.4.5.3.8). Bios wants to read N Bytes when uses this protocol and the
length specified by Bios should be greater than 1. If the Bios specified 0
bytes, the associated function(E,G read battery info) would be totally unusable.
I think such Bios can't pass through Windows certification:). From this point, I
think the check is not necessary.

If you still thought this maybe happen, I think it makes more sense to add the
check length in the ACPICA. Because ACPICA will allocate a data buffer for I2C
ACPI operation region access before call the callback. The buffer length will be
result of protocol head length plus data length. If data length is 0 and this
means the access will be invalid and ACPICA should ignore it or produce a warning.

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Mika Westerberg]

On Wed, Aug 20, 2014 at 04:59:59PM +0800, Lan Tianyu wrote:
> On 08/19/2014 11:48 PM, Mika Westerberg wrote:
> >On Tue, Aug 19, 2014 at 10:38:08AM -0500, Wolfram Sang wrote:
> >>On Tue, Aug 19, 2014 at 06:16:49PM +0300, Mika Westerberg wrote:
> >>>On Tue, Aug 19, 2014 at 10:03:55AM -0500, Wolfram Sang wrote:
> >>>>On Tue, Aug 12, 2014 at 10:33:38AM +0800, Xiubo Li wrote:
> >>>>>Since we cannot make sure the 'data_len' will always be none zero
> >>>>>here, and then if 'data_len' equals to zero, the kzalloc() will
> >>>>>return ZERO_SIZE_PTR, which equals to ((void *)16).
> >>>>
> >>>>I assume the read request with length == 0 comes from a broken BIOS?
> >>>
> >>>I'm also interested. Does this trigger in a real system?
> >>
> >>Even if not now, we should consider potentially broken BIOSes, or? Which
> >>extends the question to: Do we need even more sanity checks when taking
> >>broken BIOSes into account?
> >
> >Typically ACPICA has done this work for us (e.g it fixes things upfront so
> >that we get sane data). I'm not sure if it does that for I2C Operation
> >Regions, though (that's why I'm asking if it happens in a real system or is
> >this more like a theoretical possibility).
> >
> >Tianyu, any comments?
> >
> 
> Sorry for later response due to leave home today. acpi_gsb_i2c_read_bytes()
> dedicates for GenericSerialBus Read/Write N Bytes protocol(ACPI Spec
> 5.5.2.4.5.3.8). Bios wants to read N Bytes when uses this protocol and the
> length specified by Bios should be greater than 1. If the Bios specified 0
> bytes, the associated function(E,G read battery info) would be totally unusable.
> I think such Bios can't pass through Windows certification:). From this point, I
> think the check is not necessary.
> 
> If you still thought this maybe happen, I think it makes more sense to add the
> check length in the ACPICA. Because ACPICA will allocate a data buffer for I2C
> ACPI operation region access before call the callback. The buffer length will be
> result of protocol head length plus data length. If data length is 0 and this
> means the access will be invalid and ACPICA should ignore it or produce a warning.
> 

Thanks Tianyu for the clarification.

So Wolfram, up to you -- in principle this check is not needed but it
doesn't do any harm either.

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Wolfram Sang]

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

Hi people,

thanks for the additional information here.

> > Sorry for later response due to leave home today. acpi_gsb_i2c_read_bytes()
> > dedicates for GenericSerialBus Read/Write N Bytes protocol(ACPI Spec
> > 5.5.2.4.5.3.8). Bios wants to read N Bytes when uses this protocol and the
> > length specified by Bios should be greater than 1. If the Bios specified 0
> > bytes, the associated function(E,G read battery info) would be totally unusable.
> > I think such Bios can't pass through Windows certification:). From this point, I
> > think the check is not necessary.

The simple question behind this is: Do I trust the caller? When I look
at BIOS (or anything outside the kernel for that matter), I clearly say
no, so...

> > If you still thought this maybe happen, I think it makes more sense to add the
> > check length in the ACPICA. Because ACPICA will allocate a data buffer for I2C
> > ACPI operation region access before call the callback. The buffer length will be
> > result of protocol head length plus data length. If data length is 0 and this
> > means the access will be invalid and ACPICA should ignore it or produce a warning.

... I'd think such a check in ACPICA should be made. However, I can
still ask the question if I trust callers outside my subsystem. This is
more policy. We can demand that users of acpi_i2c_space_handler() should
sanity check their arguments. Any foreseeable chance there will be
another user other than ACPICA? I'd think no?

Regards,

   Wolfram


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

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Mika Westerberg]

On Tue, Sep 30, 2014 at 11:19:49AM +0200, Wolfram Sang wrote:
> Hi people,
> 
> thanks for the additional information here.
> 
> > > Sorry for later response due to leave home today. acpi_gsb_i2c_read_bytes()
> > > dedicates for GenericSerialBus Read/Write N Bytes protocol(ACPI Spec
> > > 5.5.2.4.5.3.8). Bios wants to read N Bytes when uses this protocol and the
> > > length specified by Bios should be greater than 1. If the Bios specified 0
> > > bytes, the associated function(E,G read battery info) would be totally unusable.
> > > I think such Bios can't pass through Windows certification:). From this point, I
> > > think the check is not necessary.
> 
> The simple question behind this is: Do I trust the caller? When I look
> at BIOS (or anything outside the kernel for that matter), I clearly say
> no, so...
> 
> > > If you still thought this maybe happen, I think it makes more sense to add the
> > > check length in the ACPICA. Because ACPICA will allocate a data buffer for I2C
> > > ACPI operation region access before call the callback. The buffer length will be
> > > result of protocol head length plus data length. If data length is 0 and this
> > > means the access will be invalid and ACPICA should ignore it or produce a warning.
> 
> ... I'd think such a check in ACPICA should be made. However, I can
> still ask the question if I trust callers outside my subsystem. This is
> more policy. We can demand that users of acpi_i2c_space_handler() should
> sanity check their arguments. Any foreseeable chance there will be
> another user other than ACPICA? I'd think no?

I'm not entirely sure I understand your question.

It is supposed to work like this:

 1. AML (firmware) code wants to do an I2C transaction. It may be
    triggered from an GPE event or something else.

 2. It reads/writes to an I2C operation region if it is available.

 3. This all is handled inside ACPICA.

 4. ACPICA calls registered address space handler for I2C which is
    acpi_i2c_space_handler().

 5. acpi_i2c_space_handler() handles the I2C transaction in the OS
    context and returns back whatever is requested to the AML (firmware)
    code.

So the only caller of acpi_i2c_space_handler() is ACPICA and we sure can
require it to validate the parameters it passes.

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Wolfram Sang]

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

> So the only caller of acpi_i2c_space_handler() is ACPICA and we sure can
> require it to validate the parameters it passes.

Fine, I'd say. Thanks!


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

Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.

[Wolfram Sang]

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

On Tue, Sep 30, 2014 at 12:35:53PM +0200, Wolfram Sang wrote:
> > So the only caller of acpi_i2c_space_handler() is ACPICA and we sure can
> > require it to validate the parameters it passes.
> 
> Fine, I'd say. Thanks!

What I meant to say by this: Yes, that should really be done in ACPICA
:)

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