i2c: core-smbus: fix a potential uninitialization bug
diff mbox series

Message ID 1525300581-27217-1-git-send-email-wang6495@umn.edu
State Superseded
Headers show
Series
  • i2c: core-smbus: fix a potential uninitialization bug
Related show

Commit Message

Wenwen Wang May 2, 2018, 10:36 p.m. UTC
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, mgsbuf1 is not always
initialized on all possible execution paths (implementation) of
i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
behaviors or security issues.

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

Comments

Peter Rosin May 3, 2018, 8:34 p.m. UTC | #1
On 2018-05-03 00:36, 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, mgsbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be

double negation here

> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
> behaviors or security issues.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/i2c/i2c-core-smbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..0fcca75 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  	 * somewhat simpler.
>  	 */
>  	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
> -	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
> +	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};

I think this will result in the whole of msgbuf1 being filled with zeroes.
It might be cheaper to do this with code proper rather than with an
initializer?

Cheers,
Peter

>  	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>  	int i;
>  	u8 partial_pec = 0;
>
Wenwen Wang May 4, 2018, 4:08 a.m. UTC | #2
On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>> initialized on all possible execution paths (implementation) of
>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>
> double negation here
>
>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>> behaviors or security issues.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>> ---
>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>> index b5aec33..0fcca75 100644
>> --- a/drivers/i2c/i2c-core-smbus.c
>> +++ b/drivers/i2c/i2c-core-smbus.c
>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>        * somewhat simpler.
>>        */
>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>
> I think this will result in the whole of msgbuf1 being filled with zeroes.
> It might be cheaper to do this with code proper rather than with an
> initializer?

Thanks for your comment, Peter!  How about using a memset() only when
i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
used only in that case?

Thanks,
Wenwen

>
> Cheers,
> Peter
>
>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>       int i;
>>       u8 partial_pec = 0;
>>
>
Peter Rosin May 4, 2018, 5:04 a.m. UTC | #3
On 2018-05-04 06:08, Wenwen Wang wrote:
> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>> initialized on all possible execution paths (implementation) of
>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>
>> double negation here
>>
>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>> behaviors or security issues.
>>>
>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>> ---
>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>> index b5aec33..0fcca75 100644
>>> --- a/drivers/i2c/i2c-core-smbus.c
>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>        * somewhat simpler.
>>>        */
>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>
>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>> It might be cheaper to do this with code proper rather than with an
>> initializer?
> 
> Thanks for your comment, Peter!  How about using a memset() only when
> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
> used only in that case?

I was thinking that an assignment of

	msgbuf1[0] = 0;

would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
cases before the i2c_transfer call. However, this will only kick in if
the call to kzalloc fails (and it most likely will not) in the call to the
i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
seems like a non-issue to me.

However, while looking I think the bigger problem with that function is that
it considers all non-negative return values from i2c_transfer as good<tm>.
IMHO, it should barf on any return values <> num. Or at the very least
describe why a partial result is considered OK...

Cheers,
Peter

>>
>> Cheers,
>> Peter
>>
>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>       int i;
>>>       u8 partial_pec = 0;
>>>
>>
Wenwen Wang May 4, 2018, 5:28 a.m. UTC | #4
On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 06:08, Wenwen Wang wrote:
>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>> initialized on all possible execution paths (implementation) of
>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>
>>> double negation here
>>>
>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>> behaviors or security issues.
>>>>
>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>> ---
>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>> index b5aec33..0fcca75 100644
>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>        * somewhat simpler.
>>>>        */
>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>
>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>> It might be cheaper to do this with code proper rather than with an
>>> initializer?
>>
>> Thanks for your comment, Peter!  How about using a memset() only when
>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>> used only in that case?
>
> I was thinking that an assignment of
>
>         msgbuf1[0] = 0;
>
> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
> cases before the i2c_transfer call. However, this will only kick in if
> the call to kzalloc fails (and it most likely will not) in the call to the
> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
> seems like a non-issue to me.
>
> However, while looking I think the bigger problem with that function is that
> it considers all non-negative return values from i2c_transfer as good<tm>.
> IMHO, it should barf on any return values <> num. Or at the very least
> describe why a partial result is considered OK...
>
> Cheers,
> Peter
>
>>>
>>> Cheers,
>>> Peter
>>>
>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>       int i;
>>>>       u8 partial_pec = 0;
>>>>
>>>
>

Yes, it is a big issue if the return value from i2c_transfer() is not
equal to num. I can add a check like this:

if (status != num)
  return -EINVAL;

Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
in i2c_smbus_try_get_dmabuf()?

Thanks,
Wenwen
Peter Rosin May 4, 2018, 6:49 a.m. UTC | #5
On 2018-05-04 07:28, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>>> initialized on all possible execution paths (implementation) of
>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>
>>>> double negation here
>>>>
>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>> behaviors or security issues.
>>>>>
>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>> ---
>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>> index b5aec33..0fcca75 100644
>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>        * somewhat simpler.
>>>>>        */
>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>
>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>> It might be cheaper to do this with code proper rather than with an
>>>> initializer?
>>>
>>> Thanks for your comment, Peter!  How about using a memset() only when
>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>> used only in that case?
>>
>> I was thinking that an assignment of
>>
>>         msgbuf1[0] = 0;
>>
>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>> cases before the i2c_transfer call. However, this will only kick in if
>> the call to kzalloc fails (and it most likely will not) in the call to the
>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>> seems like a non-issue to me.
>>
>> However, while looking I think the bigger problem with that function is that
>> it considers all non-negative return values from i2c_transfer as good<tm>.
>> IMHO, it should barf on any return values <> num. Or at the very least
>> describe why a partial result is considered OK...
>>
>> Cheers,
>> Peter
>>
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>       int i;
>>>>>       u8 partial_pec = 0;
>>>>>
>>>>
>>
> 
> Yes, it is a big issue if the return value from i2c_transfer() is not
> equal to num. I can add a check like this:
> 
> if (status != num)
>   return -EINVAL;
> 

Right, but make sure to add it *after* the existing "if (status < 0)"
check as we want to preserve any existing error. Also, -EIO is perhaps
more appropriate than -EINVAL which seems wrong for what is probably
a runtime incident.

> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
> in i2c_smbus_try_get_dmabuf()?

It is not always replaced. The stack buffer is probably retained as
the default mode of operation (and fallback) because kzalloc is
expensive and because kzalloc might fail?

Cheers,
Peter

> Thanks,
> Wenwen
>
Wenwen Wang May 4, 2018, 7:17 a.m. UTC | #6
On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 07:28, Wenwen Wang wrote:
>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>>>> initialized on all possible execution paths (implementation) of
>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>
>>>>> double negation here
>>>>>
>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>> behaviors or security issues.
>>>>>>
>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>> ---
>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>> index b5aec33..0fcca75 100644
>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>        * somewhat simpler.
>>>>>>        */
>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>
>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>> It might be cheaper to do this with code proper rather than with an
>>>>> initializer?
>>>>
>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>> used only in that case?
>>>
>>> I was thinking that an assignment of
>>>
>>>         msgbuf1[0] = 0;
>>>
>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>> cases before the i2c_transfer call. However, this will only kick in if
>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>> seems like a non-issue to me.
>>>
>>> However, while looking I think the bigger problem with that function is that
>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>> IMHO, it should barf on any return values <> num. Or at the very least
>>> describe why a partial result is considered OK...
>>>
>>> Cheers,
>>> Peter
>>>
>>>>>
>>>>> Cheers,
>>>>> Peter
>>>>>
>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>       int i;
>>>>>>       u8 partial_pec = 0;
>>>>>>
>>>>>
>>>
>>
>> Yes, it is a big issue if the return value from i2c_transfer() is not
>> equal to num. I can add a check like this:
>>
>> if (status != num)
>>   return -EINVAL;
>>
>
> Right, but make sure to add it *after* the existing "if (status < 0)"
> check as we want to preserve any existing error. Also, -EIO is perhaps
> more appropriate than -EINVAL which seems wrong for what is probably
> a runtime incident.
>

Sure, I will place it after the existing check and replace -EINVAL with -EIO.

>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>> in i2c_smbus_try_get_dmabuf()?
>
> It is not always replaced. The stack buffer is probably retained as
> the default mode of operation (and fallback) because kzalloc is
> expensive and because kzalloc might fail?
>

That means the stack buffer is probably used if kzalloc is failed.
Actually, the kzalloc failure would be possible if a user-space
process maliciously causes the kernel to consume a large chunk of
memory. In that case, the user can potentially exploit this
problematic code. So it may be better to initialize the stack buffer.

Thanks,
Wenwen

> Cheers,
> Peter
>
>> Thanks,
>> Wenwen
>>
>
Peter Rosin May 4, 2018, 7:27 a.m. UTC | #7
On 2018-05-04 09:17, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>
>>>>>> double negation here
>>>>>>
>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>> behaviors or security issues.
>>>>>>>
>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>> ---
>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>> index b5aec33..0fcca75 100644
>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>        * somewhat simpler.
>>>>>>>        */
>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>
>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>> initializer?
>>>>>
>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>> used only in that case?
>>>>
>>>> I was thinking that an assignment of
>>>>
>>>>         msgbuf1[0] = 0;
>>>>
>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>> seems like a non-issue to me.
>>>>
>>>> However, while looking I think the bigger problem with that function is that
>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>> describe why a partial result is considered OK...
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Peter
>>>>>>
>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>       int i;
>>>>>>>       u8 partial_pec = 0;
>>>>>>>
>>>>>>
>>>>
>>>
>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>> equal to num. I can add a check like this:
>>>
>>> if (status != num)
>>>   return -EINVAL;
>>>
>>
>> Right, but make sure to add it *after* the existing "if (status < 0)"
>> check as we want to preserve any existing error. Also, -EIO is perhaps
>> more appropriate than -EINVAL which seems wrong for what is probably
>> a runtime incident.
>>
> 
> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
> 
>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>> in i2c_smbus_try_get_dmabuf()?
>>
>> It is not always replaced. The stack buffer is probably retained as
>> the default mode of operation (and fallback) because kzalloc is
>> expensive and because kzalloc might fail?
>>
> 
> That means the stack buffer is probably used if kzalloc is failed.
> Actually, the kzalloc failure would be possible if a user-space
> process maliciously causes the kernel to consume a large chunk of
> memory. In that case, the user can potentially exploit this
> problematic code. So it may be better to initialize the stack buffer.

Yes, but I see little reason to initialize more than the first byte.

You hinted in the commit message that there were execution paths (or
implementations) where the second buffer wasn't initialized. Can you
give an example where this matters when the more extensive check on
the i2c_transfer return value is in place? That seems like a bugs
that should *also* be fixed in the affected i2c bus drivers...

Cheers,
Peter
Wenwen Wang May 4, 2018, 2:59 p.m. UTC | #8
On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 09:17, Wenwen Wang wrote:
>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>>
>>>>>>> double negation here
>>>>>>>
>>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>>> behaviors or security issues.
>>>>>>>>
>>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>>> ---
>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>>        * somewhat simpler.
>>>>>>>>        */
>>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>
>>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>> initializer?
>>>>>>
>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>> used only in that case?
>>>>>
>>>>> I was thinking that an assignment of
>>>>>
>>>>>         msgbuf1[0] = 0;
>>>>>
>>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>>> seems like a non-issue to me.
>>>>>
>>>>> However, while looking I think the bigger problem with that function is that
>>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>>> describe why a partial result is considered OK...
>>>>>
>>>>> Cheers,
>>>>> Peter
>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Peter
>>>>>>>
>>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>>       int i;
>>>>>>>>       u8 partial_pec = 0;
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>>> equal to num. I can add a check like this:
>>>>
>>>> if (status != num)
>>>>   return -EINVAL;
>>>>
>>>
>>> Right, but make sure to add it *after* the existing "if (status < 0)"
>>> check as we want to preserve any existing error. Also, -EIO is perhaps
>>> more appropriate than -EINVAL which seems wrong for what is probably
>>> a runtime incident.
>>>
>>
>> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
>>
>>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>>> in i2c_smbus_try_get_dmabuf()?
>>>
>>> It is not always replaced. The stack buffer is probably retained as
>>> the default mode of operation (and fallback) because kzalloc is
>>> expensive and because kzalloc might fail?
>>>
>>
>> That means the stack buffer is probably used if kzalloc is failed.
>> Actually, the kzalloc failure would be possible if a user-space
>> process maliciously causes the kernel to consume a large chunk of
>> memory. In that case, the user can potentially exploit this
>> problematic code. So it may be better to initialize the stack buffer.
>
> Yes, but I see little reason to initialize more than the first byte.
>
> You hinted in the commit message that there were execution paths (or
> implementations) where the second buffer wasn't initialized. Can you
> give an example where this matters when the more extensive check on
> the i2c_transfer return value is in place? That seems like a bugs
> that should *also* be fixed in the affected i2c bus drivers...

One possible execution path is as follows:

i2c_transfer -> __i2c_transfer -> pca_xfer (which is one of the
master_xfer handlers)

In pca_xfer(), it reads the status of the i2c_adapter and then
performs different actions according to different statuses.

It seems probable that the buffer is not filled with the wanted data
if the status is not as expected.

Wenwen
Peter Rosin May 4, 2018, 3:38 p.m. UTC | #9
On 2018-05-04 16:59, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-04 09:17, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>>>
>>>>>>>> double negation here
>>>>>>>>
>>>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>>>> behaviors or security issues.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>>>> ---
>>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>>>        * somewhat simpler.
>>>>>>>>>        */
>>>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>>
>>>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>>> initializer?
>>>>>>>
>>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>>> used only in that case?
>>>>>>
>>>>>> I was thinking that an assignment of
>>>>>>
>>>>>>         msgbuf1[0] = 0;
>>>>>>
>>>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>>>> seems like a non-issue to me.
>>>>>>
>>>>>> However, while looking I think the bigger problem with that function is that
>>>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>>>> describe why a partial result is considered OK...
>>>>>>
>>>>>> Cheers,
>>>>>> Peter
>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Peter
>>>>>>>>
>>>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>>>       int i;
>>>>>>>>>       u8 partial_pec = 0;
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>>>> equal to num. I can add a check like this:
>>>>>
>>>>> if (status != num)
>>>>>   return -EINVAL;
>>>>>
>>>>
>>>> Right, but make sure to add it *after* the existing "if (status < 0)"
>>>> check as we want to preserve any existing error. Also, -EIO is perhaps
>>>> more appropriate than -EINVAL which seems wrong for what is probably
>>>> a runtime incident.
>>>>
>>>
>>> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
>>>
>>>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>>>> in i2c_smbus_try_get_dmabuf()?
>>>>
>>>> It is not always replaced. The stack buffer is probably retained as
>>>> the default mode of operation (and fallback) because kzalloc is
>>>> expensive and because kzalloc might fail?
>>>>
>>>
>>> That means the stack buffer is probably used if kzalloc is failed.
>>> Actually, the kzalloc failure would be possible if a user-space
>>> process maliciously causes the kernel to consume a large chunk of
>>> memory. In that case, the user can potentially exploit this
>>> problematic code. So it may be better to initialize the stack buffer.
>>
>> Yes, but I see little reason to initialize more than the first byte.
>>
>> You hinted in the commit message that there were execution paths (or
>> implementations) where the second buffer wasn't initialized. Can you
>> give an example where this matters when the more extensive check on
>> the i2c_transfer return value is in place? That seems like a bugs
>> that should *also* be fixed in the affected i2c bus drivers...
> 
> One possible execution path is as follows:
> 
> i2c_transfer -> __i2c_transfer -> pca_xfer (which is one of the
> master_xfer handlers)
> 
> In pca_xfer(), it reads the status of the i2c_adapter and then
> performs different actions according to different statuses.
> 
> It seems probable that the buffer is not filled with the wanted data
> if the status is not as expected.

Ah, so you're talking about hardware malfunction without any actual
real-life incident. In other words, pure speculation. I'm sure the
kernel is full of problems if every potential HW misbehavior is
considered, and I'm not so sure this particular problem is going
to matter all that much...

Cheers,
Peter
Wenwen Wang May 5, 2018, 1:28 a.m. UTC | #10
On Fri, May 4, 2018 at 10:38 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 16:59, Wenwen Wang wrote:
>> On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-04 09:17, Wenwen Wang wrote:
>>>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>>>> On 2018-05-03 00:36, 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, mgsbuf1 is not always
>>>>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>>>>
>>>>>>>>> double negation here
>>>>>>>>>
>>>>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>>>>> behaviors or security issues.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>>>>        * somewhat simpler.
>>>>>>>>>>        */
>>>>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>>>
>>>>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>>>> initializer?
>>>>>>>>
>>>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>>>> used only in that case?
>>>>>>>
>>>>>>> I was thinking that an assignment of
>>>>>>>
>>>>>>>         msgbuf1[0] = 0;
>>>>>>>
>>>>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>>>>> seems like a non-issue to me.
>>>>>>>
>>>>>>> However, while looking I think the bigger problem with that function is that
>>>>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>>>>> describe why a partial result is considered OK...
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Peter
>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Peter
>>>>>>>>>
>>>>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>>>>       int i;
>>>>>>>>>>       u8 partial_pec = 0;
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>>>>> equal to num. I can add a check like this:
>>>>>>
>>>>>> if (status != num)
>>>>>>   return -EINVAL;
>>>>>>
>>>>>
>>>>> Right, but make sure to add it *after* the existing "if (status < 0)"
>>>>> check as we want to preserve any existing error. Also, -EIO is perhaps
>>>>> more appropriate than -EINVAL which seems wrong for what is probably
>>>>> a runtime incident.
>>>>>
>>>>
>>>> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
>>>>
>>>>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>>>>> in i2c_smbus_try_get_dmabuf()?
>>>>>
>>>>> It is not always replaced. The stack buffer is probably retained as
>>>>> the default mode of operation (and fallback) because kzalloc is
>>>>> expensive and because kzalloc might fail?
>>>>>
>>>>
>>>> That means the stack buffer is probably used if kzalloc is failed.
>>>> Actually, the kzalloc failure would be possible if a user-space
>>>> process maliciously causes the kernel to consume a large chunk of
>>>> memory. In that case, the user can potentially exploit this
>>>> problematic code. So it may be better to initialize the stack buffer.
>>>
>>> Yes, but I see little reason to initialize more than the first byte.
>>>
>>> You hinted in the commit message that there were execution paths (or
>>> implementations) where the second buffer wasn't initialized. Can you
>>> give an example where this matters when the more extensive check on
>>> the i2c_transfer return value is in place? That seems like a bugs
>>> that should *also* be fixed in the affected i2c bus drivers...
>>
>> One possible execution path is as follows:
>>
>> i2c_transfer -> __i2c_transfer -> pca_xfer (which is one of the
>> master_xfer handlers)
>>
>> In pca_xfer(), it reads the status of the i2c_adapter and then
>> performs different actions according to different statuses.
>>
>> It seems probable that the buffer is not filled with the wanted data
>> if the status is not as expected.
>
> Ah, so you're talking about hardware malfunction without any actual
> real-life incident. In other words, pure speculation. I'm sure the
> kernel is full of problems if every potential HW misbehavior is
> considered, and I'm not so sure this particular problem is going
> to matter all that much...

Thanks for your comments, Peter! I will submit a new patch :)

Wenwen

Patch
diff mbox series

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..0fcca75 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -324,7 +324,7 @@  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	 * somewhat simpler.
 	 */
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
-	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
+	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
 	int i;
 	u8 partial_pec = 0;