2015-04-17 05:40:53

by Heinrich Schuchardt

Subject: [PATCH 1/1] drivers/usb/chipidea/debuc.c: avoid out of bound read

A string written by the user may not be zero terminated.

sscanf may read memory beyond the buffer if no zero byte
is found.

Signed-off-by: Heinrich Schuchardt <[email protected]>
---
drivers/usb/chipidea/debug.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index dfb05ed..ef08af3 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
char buf[32];
int ret;

- if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+ count = min_t(size_t, sizeof(buf) - 1, count);
+ if (copy_from_user(buf, ubuf, count))
return -EFAULT;

+ /* sscanf requires a zero terminated string */
+ buf[count] = 0;
+
if (sscanf(buf, "%u", &mode) != 1)
return -EINVAL;

--
2.1.4

2015-04-21 01:35:49

by Peter Chen

Subject: Re: [PATCH 1/1] drivers/usb/chipidea/debuc.c: avoid out of bound read

On Fri, Apr 17, 2015 at 08:04:13AM +0200, Heinrich Schuchardt wrote:
> A string written by the user may not be zero terminated.
>
> sscanf may read memory beyond the buffer if no zero byte
> is found.
>
> Signed-off-by: Heinrich Schuchardt <[email protected]>
> ---
> drivers/usb/chipidea/debug.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
> index dfb05ed..ef08af3 100644
> --- a/drivers/usb/chipidea/debug.c
> +++ b/drivers/usb/chipidea/debug.c
> @@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
> char buf[32];
> int ret;
>
> - if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> + count = min_t(size_t, sizeof(buf) - 1, count);
> + if (copy_from_user(buf, ubuf, count))
> return -EFAULT;

Any reasons to change above?
>
> + /* sscanf requires a zero terminated string */
> + buf[count] = 0;
> +

I prefer using '\0'

> if (sscanf(buf, "%u", &mode) != 1)
> return -EINVAL;
>
> --
> 2.1.4
>

--

Best Regards,
Peter Chen

2015-04-21 19:26:00

by Heinrich Schuchardt

Subject: Re: [PATCH 1/1] drivers/usb/chipidea/debuc.c: avoid out of bound read

Hello Peter,

thanks for reviewing.

On 21.04.2015 03:32, Peter Chen wrote:
> On Fri, Apr 17, 2015 at 08:04:13AM +0200, Heinrich Schuchardt wrote:
>> A string written by the user may not be zero terminated.
>>
>> sscanf may read memory beyond the buffer if no zero byte
>> is found.
>>
>> Signed-off-by: Heinrich Schuchardt <[email protected]>
>> ---
>> drivers/usb/chipidea/debug.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
>> index dfb05ed..ef08af3 100644
>> --- a/drivers/usb/chipidea/debug.c
>> +++ b/drivers/usb/chipidea/debug.c
>> @@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
>> char buf[32];
>> int ret;
>>
>> - if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>> + count = min_t(size_t, sizeof(buf) - 1, count);
>> + if (copy_from_user(buf, ubuf, count))
>> return -EFAULT;
>
> Any reasons to change above?

Otherwise we would have two lines with the term
min_t(size_t, sizeof(buf) - 1, count).

This would make the code harder to read.

>>
>> + /* sscanf requires a zero terminated string */
>> + buf[count] = 0;
>> +
>
> I prefer using '\0'

If you confirm the rest of the patch is ok, I will send an updated patch.

Best regards

Heinrich

>
>> if (sscanf(buf, "%u", &mode) != 1)
>> return -EINVAL;
>>

2015-04-22 01:29:30

by Peter Chen

Subject: Re: [PATCH 1/1] drivers/usb/chipidea/debuc.c: avoid out of bound read

On Tue, Apr 21, 2015 at 09:25:41PM +0200, Heinrich Schuchardt wrote:
> Hello Peter,
>
> thanks for reviewing.
>
> On 21.04.2015 03:32, Peter Chen wrote:
> > On Fri, Apr 17, 2015 at 08:04:13AM +0200, Heinrich Schuchardt wrote:
> >> A string written by the user may not be zero terminated.
> >>
> >> sscanf may read memory beyond the buffer if no zero byte
> >> is found.
> >>
> >> Signed-off-by: Heinrich Schuchardt <[email protected]>
> >> ---
> >> drivers/usb/chipidea/debug.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
> >> index dfb05ed..ef08af3 100644
> >> --- a/drivers/usb/chipidea/debug.c
> >> +++ b/drivers/usb/chipidea/debug.c
> >> @@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
> >> char buf[32];
> >> int ret;
> >>
> >> - if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> >> + count = min_t(size_t, sizeof(buf) - 1, count);
> >> + if (copy_from_user(buf, ubuf, count))
> >> return -EFAULT;
> >
> > Any reasons to change above?
>
> Otherwise we would have two lines with the term
> min_t(size_t, sizeof(buf) - 1, count).

Sorry, two lines of min_t(..)? Why I can't find it?


>
> This would make the code harder to read.
>
> >>
> >> + /* sscanf requires a zero terminated string */
> >> + buf[count] = 0;
> >> +
> >
> > I prefer using '\0'
>
> If you confirm the rest of the patch is ok, I will send an updated patch.
>
> Best regards
>
> Heinrich
>
> >
> >> if (sscanf(buf, "%u", &mode) != 1)
> >> return -EINVAL;
> >>
>

--

Best Regards,
Peter Chen

2015-04-27 18:25:27

by Heinrich Schuchardt

Subject: Re: [PATCH 1/1] drivers/usb/chipidea/debuc.c: avoid out of bound read

On 22.04.2015 03:26, Peter Chen wrote:
> On Tue, Apr 21, 2015 at 09:25:41PM +0200, Heinrich Schuchardt wrote:
>> Hello Peter,
>>
>> thanks for reviewing.
>>
>> On 21.04.2015 03:32, Peter Chen wrote:
>>> On Fri, Apr 17, 2015 at 08:04:13AM +0200, Heinrich Schuchardt wrote:
>>>> A string written by the user may not be zero terminated.
>>>>
>>>> sscanf may read memory beyond the buffer if no zero byte
>>>> is found.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <[email protected]>
>>>> ---
>>>> drivers/usb/chipidea/debug.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
>>>> index dfb05ed..ef08af3 100644
>>>> --- a/drivers/usb/chipidea/debug.c
>>>> +++ b/drivers/usb/chipidea/debug.c
>>>> @@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
>>>> char buf[32];
>>>> int ret;
>>>>
>>>> - if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>>>> + count = min_t(size_t, sizeof(buf) - 1, count);
>>>> + if (copy_from_user(buf, ubuf, count))
>>>> return -EFAULT;
>>>
>>> Any reasons to change above?
>>
>> Otherwise we would have two lines with the term
>> min_t(size_t, sizeof(buf) - 1, count).
>
> Sorry, two lines of min_t(..)? Why I can't find it?

Hello Peter,

in my patch I write:

count = min_t(size_t, sizeof(buf) - 1, count);
if (copy_from_user(buf, ubuf, count))
return -EFAULT;

/* sscanf requires a zero terminated string */
buf[count] = 0;

Without the first part of the change I would have had to write:

if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
return -EFAULT;

/* sscanf requires a zero terminated string */
buf[min_t(size_t, sizeof(buf) - 1, count)] = 0;

We should do the same calculation
"min_t(size_t, sizeof(buf) - 1, count)"
only once in the coding.

Best regards

Heinrich


>
>
>>
>> This would make the code harder to read.
>>
>>>>
>>>> + /* sscanf requires a zero terminated string */
>>>> + buf[count] = 0;
>>>> +
>>>
>>> I prefer using '\0'
>>
>> If you confirm the rest of the patch is ok, I will send an updated patch.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> if (sscanf(buf, "%u", &mode) != 1)
>>>> return -EINVAL;
>>>>
>>
>

2015-04-28 08:25:37

by Peter Chen

Subject: Re: [PATCH 1/1] drivers/usb/chipidea/debuc.c: avoid out of bound read

On Mon, Apr 27, 2015 at 08:25:12PM +0200, Heinrich Schuchardt wrote:
> On 22.04.2015 03:26, Peter Chen wrote:
> > On Tue, Apr 21, 2015 at 09:25:41PM +0200, Heinrich Schuchardt wrote:
> >> Hello Peter,
> >>
> >> thanks for reviewing.
> >>
> >> On 21.04.2015 03:32, Peter Chen wrote:
> >>> On Fri, Apr 17, 2015 at 08:04:13AM +0200, Heinrich Schuchardt wrote:
> >>>> A string written by the user may not be zero terminated.
> >>>>
> >>>> sscanf may read memory beyond the buffer if no zero byte
> >>>> is found.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <[email protected]>
> >>>> ---
> >>>> drivers/usb/chipidea/debug.c | 6 +++++-
> >>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
> >>>> index dfb05ed..ef08af3 100644
> >>>> --- a/drivers/usb/chipidea/debug.c
> >>>> +++ b/drivers/usb/chipidea/debug.c
> >>>> @@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
> >>>> char buf[32];
> >>>> int ret;
> >>>>
> >>>> - if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> >>>> + count = min_t(size_t, sizeof(buf) - 1, count);
> >>>> + if (copy_from_user(buf, ubuf, count))
> >>>> return -EFAULT;
> >>>
> >>> Any reasons to change above?
> >>
> >> Otherwise we would have two lines with the term
> >> min_t(size_t, sizeof(buf) - 1, count).
> >
> > Sorry, two lines of min_t(..)? Why I can't find it?
>
> Hello Peter,
>
> in my patch I write:
>
> count = min_t(size_t, sizeof(buf) - 1, count);
> if (copy_from_user(buf, ubuf, count))
> return -EFAULT;
>
> /* sscanf requires a zero terminated string */
> buf[count] = 0;
>
> Without the first part of the change I would have had to write:
>
> if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> return -EFAULT;
>
> /* sscanf requires a zero terminated string */
> buf[min_t(size_t, sizeof(buf) - 1, count)] = 0;
>
> We should do the same calculation
> "min_t(size_t, sizeof(buf) - 1, count)"
> only once in the coding.

Oh, yeah.
Send your v2 with '\0' change, thanks.

Peter

>
> Best regards
>
> Heinrich
>
>
> >
> >
> >>
> >> This would make the code harder to read.
> >>
> >>>>
> >>>> + /* sscanf requires a zero terminated string */
> >>>> + buf[count] = 0;
> >>>> +
> >>>
> >>> I prefer using '\0'
> >>
> >> If you confirm the rest of the patch is ok, I will send an updated patch.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>> if (sscanf(buf, "%u", &mode) != 1)
> >>>> return -EINVAL;
> >>>>
> >>
> >
>

--

Best Regards,
Peter Chen

2015-04-28 17:32:24

by Heinrich Schuchardt

Subject: [PATCH 1/1 v2] drivers/usb/chipidea/debuc.c: avoid out of bound read

A string written by the user may not be zero terminated.

sscanf may read memory beyond the buffer if no zero byte
is found.

For testing build with CONFIG_USB_CHIPIDEA=y, CONFIG_USB_CHIPIDEA_DEBUG=y.

Version 2:
Use '\0' for char constant.

Signed-off-by: Heinrich Schuchardt <[email protected]>
---
drivers/usb/chipidea/debug.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index dfb05ed..ef08af3 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -88,9 +88,13 @@ static ssize_t ci_port_test_write(struct file *file, const char __user *ubuf,
char buf[32];
int ret;

- if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+ count = min_t(size_t, sizeof(buf) - 1, count);
+ if (copy_from_user(buf, ubuf, count))
return -EFAULT;

+ /* sscanf requires a zero terminated string */
+ buf[count] = '\0';
+
if (sscanf(buf, "%u", &mode) != 1)
return -EINVAL;

--
2.1.4