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

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

[Heinrich Schuchardt]

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 <xypron.glpk@gmx.de>
---
 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

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

[Peter Chen]

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 <xypron.glpk@gmx.de>
> ---
>  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

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

[Heinrich Schuchardt]

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 <xypron.glpk@gmx.de>
>> ---
>>  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;
>>  

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

[Peter Chen]

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 <xypron.glpk@gmx.de>
> >> ---
> >>  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

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

[Heinrich Schuchardt]

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 <xypron.glpk@gmx.de>
>>>> ---
>>>>  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;
>>>>  
>>
> 

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

[Peter Chen]

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 <xypron.glpk@gmx.de>
> >>>> ---
> >>>>  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

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

[Heinrich Schuchardt]

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 <xypron.glpk@gmx.de>
---
 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