* [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
@ 2014-08-12 2:33 Xiubo Li
[not found] ` <1407810818-33672-1-git-send-email-Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Xiubo Li @ 2014-08-12 2:33 UTC (permalink / raw
To: wsa-z923LK4zBo2bacvFa/9K2g, linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <1407810818-33672-1-git-send-email-Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
@ 2014-08-19 15:03 ` Wolfram Sang
2014-08-19 15:16 ` Mika Westerberg
0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2014-08-19 15:03 UTC (permalink / raw
To: Xiubo Li; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Mika Westerberg, Lan Tianyu
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
2014-08-19 15:03 ` Wolfram Sang
@ 2014-08-19 15:16 ` Mika Westerberg
[not found] ` <20140819151604.GU1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2014-08-19 15:16 UTC (permalink / raw
To: Wolfram Sang; +Cc: Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Lan Tianyu
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
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <20140819151604.GU1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
@ 2014-08-19 15:38 ` Wolfram Sang
2014-08-19 15:48 ` Mika Westerberg
0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2014-08-19 15:38 UTC (permalink / raw
To: Mika Westerberg; +Cc: Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Lan Tianyu
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
2014-08-19 15:38 ` Wolfram Sang
@ 2014-08-19 15:48 ` Mika Westerberg
[not found] ` <20140819154555.GW1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2014-08-19 15:48 UTC (permalink / raw
To: Wolfram Sang; +Cc: Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Lan Tianyu
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <20140819154555.GW1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
@ 2014-08-20 2:37 ` Li.Xiubo-KZfg59tc24xl57MIdRCFDg
[not found] ` <1ff2414e255d4d978705c16339b8a586-swgC6WJTr6EbUgZD/0KOGpwN6zqB+hSMnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2014-08-20 8:59 ` Lan Tianyu
1 sibling, 1 reply; 13+ messages in thread
From: Li.Xiubo-KZfg59tc24xl57MIdRCFDg @ 2014-08-20 2:37 UTC (permalink / raw
To: Mika Westerberg, Wolfram Sang
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Lan Tianyu
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <1ff2414e255d4d978705c16339b8a586-swgC6WJTr6EbUgZD/0KOGpwN6zqB+hSMnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
@ 2014-08-20 8:00 ` Mika Westerberg
0 siblings, 0 replies; 13+ messages in thread
From: Mika Westerberg @ 2014-08-20 8:00 UTC (permalink / raw
To: Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org
Cc: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Lan Tianyu, Lv Zheng
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <20140819154555.GW1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
2014-08-20 2:37 ` Li.Xiubo-KZfg59tc24xl57MIdRCFDg
@ 2014-08-20 8:59 ` Lan Tianyu
[not found] ` <53F4638F.5070704-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
1 sibling, 1 reply; 13+ messages in thread
From: Lan Tianyu @ 2014-08-20 8:59 UTC (permalink / raw
To: Mika Westerberg, Wolfram Sang, Xiubo Li
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Zheng, Lv
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <53F4638F.5070704-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2014-08-20 10:18 ` Mika Westerberg
[not found] ` <20140820101814.GC1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2014-08-20 10:18 UTC (permalink / raw
To: Lan Tianyu
Cc: Wolfram Sang, Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Zheng, Lv
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <20140820101814.GC1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
@ 2014-09-30 9:19 ` Wolfram Sang
2014-09-30 9:40 ` Mika Westerberg
0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2014-09-30 9:19 UTC (permalink / raw
To: Mika Westerberg
Cc: Lan Tianyu, Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Zheng, Lv
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
2014-09-30 9:19 ` Wolfram Sang
@ 2014-09-30 9:40 ` Mika Westerberg
[not found] ` <20140930094008.GP1786-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2014-09-30 9:40 UTC (permalink / raw
To: Wolfram Sang
Cc: Lan Tianyu, Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Zheng, Lv
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
[not found] ` <20140930094008.GP1786-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
@ 2014-09-30 10:35 ` Wolfram Sang
2014-10-03 0:55 ` Wolfram Sang
0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2014-09-30 10:35 UTC (permalink / raw
To: Mika Westerberg
Cc: Lan Tianyu, Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Zheng, Lv
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error.
2014-09-30 10:35 ` Wolfram Sang
@ 2014-10-03 0:55 ` Wolfram Sang
0 siblings, 0 replies; 13+ messages in thread
From: Wolfram Sang @ 2014-10-03 0:55 UTC (permalink / raw
To: Mika Westerberg
Cc: Lan Tianyu, Xiubo Li, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Zheng, Lv
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-10-03 0:55 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-12 2:33 [PATCH] I2C/ACPI: Fix possible ZERO_SIZE_PTR pointer dereferencing error Xiubo Li
[not found] ` <1407810818-33672-1-git-send-email-Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2014-08-19 15:03 ` Wolfram Sang
2014-08-19 15:16 ` Mika Westerberg
[not found] ` <20140819151604.GU1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
2014-08-19 15:38 ` Wolfram Sang
2014-08-19 15:48 ` Mika Westerberg
[not found] ` <20140819154555.GW1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
2014-08-20 2:37 ` Li.Xiubo-KZfg59tc24xl57MIdRCFDg
[not found] ` <1ff2414e255d4d978705c16339b8a586-swgC6WJTr6EbUgZD/0KOGpwN6zqB+hSMnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2014-08-20 8:00 ` Mika Westerberg
2014-08-20 8:59 ` Lan Tianyu
[not found] ` <53F4638F.5070704-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2014-08-20 10:18 ` Mika Westerberg
[not found] ` <20140820101814.GC1660-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
2014-09-30 9:19 ` Wolfram Sang
2014-09-30 9:40 ` Mika Westerberg
[not found] ` <20140930094008.GP1786-3PARRvDOhMZrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
2014-09-30 10:35 ` Wolfram Sang
2014-10-03 0:55 ` Wolfram Sang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.