| Message ID | 1407810818-33672-1-git-send-email-Li.Xiubo@freescale.com |
|---|---|
| State | Rejected |
| Headers | show |
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@freescale.com> 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 >
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@freescale.com> > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 20, 2014 at 02:37:57AM +0000, Li.Xiubo@freescale.com 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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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!
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 :)
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;
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@freescale.com> --- drivers/i2c/i2c-acpi.c | 3 +++ 1 file changed, 3 insertions(+)