All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] i2c: core-smbus: fix a potential uninitialization bug
@ 2018-05-05 12:57 ` Wenwen Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Wenwen Wang @ 2018-05-05 12:57 UTC (permalink / raw)
  To: Wenwen Wang; +Cc: Kangjie Lu, Wolfram Sang, open list:I2C SUBSYSTEM, open list

In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
which are used to save a series of messages, as mentioned in the comment.
According to the value of the variable 'size', msgbuf0 is initialized to
various values. In contrast, msgbuf1 is left uninitialized until the
function i2c_transfer() is invoked. However, msgbuf1 is not always
initialized on all possible execution paths (implementation) of
i2c_transfer(). Thus, it is possible that msgbuf1 may still be
uninitialized even after the invocation of the function i2c_transfer(),
especially when the return value of ic2_transfer() is not checked properly.
In the following execution, the uninitialized msgbuf1 will be used, such as
for security checks. Since uninitialized values can be random and
arbitrary, this will cause undefined behaviors or even check bypass. For
example, it is expected that if the value of 'size' is
I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
value read from msgbuf1 is assigned to data->block[0], which can
potentially lead to invalid block write size, as demonstrated in the error
message.

This patch initializes the first byte of msgbuf1 with 0 to avoid such
undefined behaviors or security issues.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/i2c/i2c-core-smbus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..7d7700f 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -344,6 +344,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	};
 
 	msgbuf0[0] = command;
+	msgbuf1[0] = 0;
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		msg[0].len = 0;
-- 
2.7.4

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

* [PATCH v2 1/2] i2c: core-smbus: fix a potential uninitialization bug
@ 2018-05-05 12:57 ` Wenwen Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Wenwen Wang @ 2018-05-05 12:57 UTC (permalink / raw)
  To: Wenwen Wang; +Cc: Kangjie Lu, Wolfram Sang, open list:I2C SUBSYSTEM, open list

In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
which are used to save a series of messages, as mentioned in the comment.
According to the value of the variable 'size', msgbuf0 is initialized to
various values. In contrast, msgbuf1 is left uninitialized until the
function i2c_transfer() is invoked. However, msgbuf1 is not always
initialized on all possible execution paths (implementation) of
i2c_transfer(). Thus, it is possible that msgbuf1 may still be
uninitialized even after the invocation of the function i2c_transfer(),
especially when the return value of ic2_transfer() is not checked properly.
In the following execution, the uninitialized msgbuf1 will be used, such as
for security checks. Since uninitialized values can be random and
arbitrary, this will cause undefined behaviors or even check bypass. For
example, it is expected that if the value of 'size' is
I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
value read from msgbuf1 is assigned to data->block[0], which can
potentially lead to invalid block write size, as demonstrated in the error
message.

This patch initializes the first byte of msgbuf1 with 0 to avoid such
undefined behaviors or security issues.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/i2c/i2c-core-smbus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..7d7700f 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -344,6 +344,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	};
 
 	msgbuf0[0] = command;
+	msgbuf1[0] = 0;
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		msg[0].len = 0;
-- 
2.7.4

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

* Re: [PATCH v2 1/2] i2c: core-smbus: fix a potential uninitialization bug
  2018-05-05 12:57 ` Wenwen Wang
  (?)
@ 2018-05-10 11:17 ` Wolfram Sang
  2018-05-14 20:31   ` Peter Rosin
  -1 siblings, 1 reply; 5+ messages in thread
From: Wolfram Sang @ 2018-05-10 11:17 UTC (permalink / raw)
  To: Wenwen Wang; +Cc: Kangjie Lu, open list:I2C SUBSYSTEM, open list

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

On Sat, May 05, 2018 at 07:57:10AM -0500, Wenwen Wang wrote:
> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
> which are used to save a series of messages, as mentioned in the comment.
> According to the value of the variable 'size', msgbuf0 is initialized to
> various values. In contrast, msgbuf1 is left uninitialized until the
> function i2c_transfer() is invoked. However, msgbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that msgbuf1 may still be
> uninitialized even after the invocation of the function i2c_transfer(),
> especially when the return value of ic2_transfer() is not checked properly.
> In the following execution, the uninitialized msgbuf1 will be used, such as
> for security checks. Since uninitialized values can be random and
> arbitrary, this will cause undefined behaviors or even check bypass. For
> example, it is expected that if the value of 'size' is
> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
> value read from msgbuf1 is assigned to data->block[0], which can
> potentially lead to invalid block write size, as demonstrated in the error
> message.
> 
> This patch initializes the first byte of msgbuf1 with 0 to avoid such
> undefined behaviors or security issues.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>

From what I can tell, this patch is not needed anymore after patch 2 is
applied. Correct?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH v2 1/2] i2c: core-smbus: fix a potential uninitialization bug
  2018-05-10 11:17 ` Wolfram Sang
@ 2018-05-14 20:31   ` Peter Rosin
  2018-05-18 19:25     ` Wenwen Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Peter Rosin @ 2018-05-14 20:31 UTC (permalink / raw)
  To: Wolfram Sang, Wenwen Wang; +Cc: Kangjie Lu, open list:I2C SUBSYSTEM, open list

On 2018-05-10 13:17, Wolfram Sang wrote:
> On Sat, May 05, 2018 at 07:57:10AM -0500, Wenwen Wang wrote:
>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>> which are used to save a series of messages, as mentioned in the comment.
>> According to the value of the variable 'size', msgbuf0 is initialized to
>> various values. In contrast, msgbuf1 is left uninitialized until the
>> function i2c_transfer() is invoked. However, msgbuf1 is not always
>> initialized on all possible execution paths (implementation) of
>> i2c_transfer(). Thus, it is possible that msgbuf1 may still be
>> uninitialized even after the invocation of the function i2c_transfer(),
>> especially when the return value of ic2_transfer() is not checked properly.
>> In the following execution, the uninitialized msgbuf1 will be used, such as
>> for security checks. Since uninitialized values can be random and
>> arbitrary, this will cause undefined behaviors or even check bypass. For
>> example, it is expected that if the value of 'size' is
>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
>> value read from msgbuf1 is assigned to data->block[0], which can
>> potentially lead to invalid block write size, as demonstrated in the error
>> message.
>>
>> This patch initializes the first byte of msgbuf1 with 0 to avoid such
>> undefined behaviors or security issues.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> 
> From what I can tell, this patch is not needed anymore after patch 2 is
> applied. Correct?

AFAIU, it is only needed if there are bugs elsewhere. I.e. it's for extra
protection. If all drivers implement .master_xfer correctly, msgbuf1 will
be filled in and the return value will be the number of messages (i.e. 2)
OR you get a negative return value and the msgbuf1 content will not matter.

The patch does not magically fix all possible driver bugs, so in that
sense this patch is still "needed".

Also - again AFAIU - there is no known bug that actually gets caught by
this extra check.

Cheers,
Peter

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

* Re: [PATCH v2 1/2] i2c: core-smbus: fix a potential uninitialization bug
  2018-05-14 20:31   ` Peter Rosin
@ 2018-05-18 19:25     ` Wenwen Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Wenwen Wang @ 2018-05-18 19:25 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Peter Rosin, Kangjie Lu, open list:I2C SUBSYSTEM, open list, Wenwen Wang

Yes, this patch does not aim to "fix" all potential driver bugs but
adds an additional protection in case the implementation of
.master_xfer is incorrect.

>From this perspective, it is still necessary to apply this patch, as
pointed out by Peter.

Thanks,
Wenwen

On Mon, May 14, 2018 at 3:31 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-10 13:17, Wolfram Sang wrote:
>> On Sat, May 05, 2018 at 07:57:10AM -0500, Wenwen Wang wrote:
>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>>> which are used to save a series of messages, as mentioned in the comment.
>>> According to the value of the variable 'size', msgbuf0 is initialized to
>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>> function i2c_transfer() is invoked. However, msgbuf1 is not always
>>> initialized on all possible execution paths (implementation) of
>>> i2c_transfer(). Thus, it is possible that msgbuf1 may still be
>>> uninitialized even after the invocation of the function i2c_transfer(),
>>> especially when the return value of ic2_transfer() is not checked properly.
>>> In the following execution, the uninitialized msgbuf1 will be used, such as
>>> for security checks. Since uninitialized values can be random and
>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>> example, it is expected that if the value of 'size' is
>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
>>> value read from msgbuf1 is assigned to data->block[0], which can
>>> potentially lead to invalid block write size, as demonstrated in the error
>>> message.
>>>
>>> This patch initializes the first byte of msgbuf1 with 0 to avoid such
>>> undefined behaviors or security issues.
>>>
>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>
>> From what I can tell, this patch is not needed anymore after patch 2 is
>> applied. Correct?
>
> AFAIU, it is only needed if there are bugs elsewhere. I.e. it's for extra
> protection. If all drivers implement .master_xfer correctly, msgbuf1 will
> be filled in and the return value will be the number of messages (i.e. 2)
> OR you get a negative return value and the msgbuf1 content will not matter.
>
> The patch does not magically fix all possible driver bugs, so in that
> sense this patch is still "needed".
>
> Also - again AFAIU - there is no known bug that actually gets caught by
> this extra check.
>
> Cheers,
> Peter

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

end of thread, other threads:[~2018-05-18 19:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-05 12:57 [PATCH v2 1/2] i2c: core-smbus: fix a potential uninitialization bug Wenwen Wang
2018-05-05 12:57 ` Wenwen Wang
2018-05-10 11:17 ` Wolfram Sang
2018-05-14 20:31   ` Peter Rosin
2018-05-18 19:25     ` Wenwen Wang

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.