Search Linux Wireless

Re: [PATCH] rt2x00: unterminated strlen of user data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 18, 2016 at 5:49 AM, Julian Calaby <julian.calaby@xxxxxxxxx> wrote:
> Hi Souptick,
>
> On Thu, Feb 18, 2016 at 12:49 AM, Souptick Joarder <jrdr.linux@xxxxxxxxx> wrote:
>> Hi Julian,
>>
>> On Wed, Feb 17, 2016 at 5:38 AM, Julian Calaby <julian.calaby@xxxxxxxxx> wrote:
>>> Hi Souptick,
>>>
>>> On Tue, Feb 16, 2016 at 5:36 PM, Souptick Joarder <jrdr.linux@xxxxxxxxx> wrote:
>>>> On Tue, Feb 16, 2016 at 12:34 AM, Alan <gnomes@xxxxxxxxxxxxxxxxxxx> wrote:
>>>>> The buffer needs to be zero terminated in case the user data is not.
>>>>> Otherwise we run off the end of the buffer.
>>>>>
>>>>> Signed-off-by: Alan Cox <alan@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/net/wireless/ralink/rt2x00/rt2x00debug.c |    5 +++--
>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
>>>>> index 25ee3cb..72ae530 100644
>>>>> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
>>>>> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
>>>>> @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file,        \
>>>>>  {                                                              \
>>>>>         struct rt2x00debug_intf *intf = file->private_data;     \
>>>>>         const struct rt2x00debug *debug = intf->debug;          \
>>>>> -       char line[16];                                          \
>>>>> +       char line[17];                                          \
>>>>>         size_t size;                                            \
>>>>>         unsigned int index = intf->offset_##__name;             \
>>>>>         __type value;                                           \
>>>>> @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file,        \
>>>>>                                                                 \
>>>>>         if (copy_from_user(line, buf, length))                  \
>>>>>                 return -EFAULT;                                 \
>>>>> -                                                               \
>>>>> +       line[16] = 0;
>>>>
>>>>             line[length] = '\0';
>>>>             correct me if I am wrong.                          \
>>>
>>> I believe that in this case the data in buf will already be null
>>> terminated, so ensuring that line is null terminated is only needed if
>>> there are exactly 16 bytes in buf. IMHO
>>>
>>> line[16] = 0;
>>
>> I think, if there are 16 bytes in buf, we end up loosing 1 byte data.
>> If there are 15 bytes in buf, buf is already be null terminated.
>> So is this really required?
>> Correct me if I am wrong.
>
> Counting in C starts at 0.
>
> We started out with line being a 16 character string, so it initially
> looked like this:
>
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 <- index
> _ _ _ _ _ _ _ _ _ _ __ __ __ __ __ __ <- data, i.e. line[index]
>
> If we are given a 4 byte null terminated string, we end up with:
>
> 0 1 2 3  4 5 6 7 8 9 10 11 12 13 14 15
> a b c d \0 _ _ _ _ _ __ __ __ __ __ __
>
> If we are given a 15 byte null terminated string, we end up with:
>
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> a b c d e f g h i j  k  l  m  n  o \0
>
> If we are given a 16 byte null terminated string, we end up with:
>
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> a b c d e f g h i j  k  l  m  n  o  p
>
> Note that it is not null terminated. The call to strlen() will now run
> off into memory that doesn't belong to line and potentially cause a
> crash. This is a bug.
>
> Alan fixed this by adding another character to the string, so it's now
> 17 characters long and line[16] will always be set to 0 after
> copy_from_user() is called. So now line initially looks like:
>
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
> _ _ _ _ _ _ _ _ _ _ __ __ __ __ __ __ __
>
> If we are given a 4 byte null terminated string, we end up with:
>
> 0 1 2 3  4 5 6 7 8 9 10 11 12 13 14 15 16
> a b c d \0 _ _ _ _ _ __ __ __ __ __ __ \0
>
> If we are given a 15 byte null terminated string, we end up with:
>
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
> a b c d e f g h i j  k  l  m  n  o \0 \0
>
> If we are given a 16 byte null terminated string, we end up with:
>
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
> a b c d e f g h i j  k  l  m  n  o  p \0
>
> Note that the final null terminator is set by the line[16] = 0; line,
> not copy_from_user().
>
> As line is now null terminated, strlen() will behave correctly and
> there is no bug.
>
> There are a few different ways to fix this bug. All of them will
> involve increasing the size of line to 17, and the exact details of
> how we ensure the null terminator is set can vary. Alan has chosen
> what is likely to be the smallest option in compiled code size, the
> fastest, and one of the most obvious options.
>
> Understanding how arrays and strings work in C is essential knowledge
> if you're going to submit or review patches for the Linux kernel.
> Could you please refresh your knowledge on those topics before
> submitting or reviewing any more patches?
    Sorry, error from my side. :(
    Thanks for clarification.

> Thanks,
>
> --
> Julian Calaby
>
> Email: julian.calaby@xxxxxxxxx
> Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux