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.g...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
