Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 09:17, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@xxxxxxxxxx> wrote:
>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@xxxxxxxxxx> wrote:
>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@xxxxxxxxxx> 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@xxxxxxx>
>>>>>>> ---
>>>>>>>  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