Revert "HID: uhid: use strlcpy() instead of strncpy()"
Subsystems (2)
PATCH 1/1 Revert "HID: uhid: use strlcpy() instead of strncpy()"
This reverts commit 336fd4f5f25157e9e8bd50e898a1bbcd99eaea46.
Please note that strlcpy() does *NOT* do what you think it does. strlcpy() *ALWAYS* reads the full input string, regardless of the 'length' parameter. That is, if the input is not zero-terminated, strlcpy() will *READ* beyond input boundaries. It does this, because it always returns the size it *would* copy if the target was big enough, not the truncated size it actually copied.
The original code was perfectly fine. The hid device is zero-initialized and the strncpy() functions copied up to n-1 characters. The result is always zero-terminated this way.
This is the third time someone tried to replace strncpy with strlcpy in this function, and gets it wrong. I now added a comment that should at least make people reconsider.
--- drivers/hid/uhid.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/hid/uhid.c +7 -6
@@ -496,12 +496,13 @@ static int uhid_dev_create2(struct uhid_device *uhid, goto err_free; } - len =min (sizeof (hid->name),sizeof (ev->u.create2.name)); -strlcpy (hid->name, ev->u.create2.name, len); - len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys)); -strlcpy (hid->phys, ev->u.create2.phys, len); - len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)); -strlcpy (hid->uniq, ev->u.create2.uniq, len); +/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */ + len =min (sizeof (hid->name),sizeof (ev->u.create2.name)) -1 ; +strncpy (hid->name, ev->u.create2.name, len); + len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys)) -1 ; +strncpy (hid->phys, ev->u.create2.phys, len); + len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)) -1 ; +strncpy (hid->uniq, ev->u.create2.uniq, len); hid->ll_driver = &uhid_hid_driver; hid->bus = ev->u.create2.bus;
Signature · 1 line
2.19.1
Quoted 9 lines · David Herrmann
This reverts commit
336fd4f5f25157e9e8bd50e898a1bbcd99eaea46.Please note that
strlcpy()does *NOT* do what you think it does.strlcpy()*ALWAYS* reads the full input string, regardless of the 'length' parameter. That is, if the input is not zero-terminated,strlcpy()will *READ* beyond input boundaries. It does this, because it always returns the size it *would* copy if the target was big enough, not the truncated size it actually copied.
Quoted 8 lines
The original code was perfectly fine. The hid device is zero-initialized and the
strncpy()functions copied up to n-1 characters. The result is always zero-terminated this way.This is the third time someone tried to replace strncpy with strlcpy in this function, and gets it wrong. I now added a comment that should at least make people reconsider.
Can we switch to strscpy instead? This will quiet gcc and avoid the issues with strlcpy.
Quoted 30 lines
Signed-off-by David Herrmann <[email protected]>--- drivers/hid/uhid.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)drivers/hid/uhid.c +7
-6@@ -496,12 +496,13 @@ static int uhid_dev_create2(struct uhid_device *uhid, goto err_free; } - len =min (sizeof (hid->name),sizeof (ev->u.create2.name)); -strlcpy (hid->name, ev->u.create2.name, len); - len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys)); -strlcpy (hid->phys, ev->u.create2.phys, len); - len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)); -strlcpy (hid->uniq, ev->u.create2.uniq, len); +/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */ + len =min (sizeof (hid->name),sizeof (ev->u.create2.name)) -1 ; +strncpy (hid->name, ev->u.create2.name, len); + len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys)) -1 ; +strncpy (hid->phys, ev->u.create2.phys, len); + len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)) -1 ; +strncpy (hid->uniq, ev->u.create2.uniq, len); hid->ll_driver = &uhid_hid_driver; hid->bus = ev->u.create2.bus;
Quoted 15 lines · Laura Abbott
Quoted 14 lines · David Herrmann
This reverts commit
336fd4f5f25157e9e8bd50e898a1bbcd99eaea46.Please note that
strlcpy()does *NOT* do what you think it does.strlcpy()*ALWAYS* reads the full input string, regardless of the 'length' parameter. That is, if the input is not zero-terminated,strlcpy()will *READ* beyond input boundaries. It does this, because it always returns the size it *would* copy if the target was big enough, not the truncated size it actually copied.The original code was perfectly fine. The hid device is zero-initialized and the
strncpy()functions copied up to n-1 characters. The result is always zero-terminated this way.
Quoted 7 lines
Quoted 4 lines
This is the third time someone tried to replace strncpy with strlcpy in this function, and gets it wrong. I now added a comment that should at least make people reconsider.
Can we switch to strscpy instead? This will quiet gcc and avoid the issues with strlcpy.
Yes please: it looks like these strings are expected to be NUL terminated, so strscpy() without the "- 1" and min() logic would be the correct solution here. If @hid is already zero, then this would just be:
strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));If they are NOT NUL terminated, then keep using strncpy() but mark the fields in the struct with the __nonstring attribute.
-Kees
Quoted 33 lines
Quoted 32 lines
Signed-off-by David Herrmann <[email protected]>--- drivers/hid/uhid.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)drivers/hid/uhid.c +7
-5@@ -496,12 +496,13 @@ static int uhid_dev_create2(struct uhid_device *uhid, goto err_free; } - len =min (sizeof (hid->name),sizeof (ev->u.create2.name)); -strlcpy(hid->name, ev->u.create2.name, len); -len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)); -strlcpy(hid->phys, ev->u.create2.phys, len); -len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)); -strlcpy(hid->uniq, ev->u.create2.uniq, len); +/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */ + len =min (sizeof (hid->name),sizeof (ev->u.create2.name)) -1 ; +strncpy (hid->name, ev->u.create2.name, len); + len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys)) -1 ; +strncpy (hid->phys, ev->u.create2.phys, len); + len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)) -1 ; +strncpy (hid->uniq, ev->u.create2.uniq, len); hid->ll_driver = &uhid_hid_driver; hid->bus = ev->u.create2.bus;
Signature · 1 line
Kees Cook
Hi
Quoted 1 line · Kees Cook
On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <[email protected]> wrote:
[...]
Quoted 6 lines
Quoted 2 lines
Can we switch to strscpy instead? This will quiet gcc and avoid the issues with strlcpy.
Yes please: it looks like these strings are expected to be NUL terminated, so
strscpy()without the "- 1" andmin()logic would be the correct solution here.
"the correct solution"? To my knowledge the original code was correct as well. Am I missing something?
Quoted 9 lines
If @hid is already zero, then this would
just be:strscpy(hid->name, ev->u.create2.name, sizeof(hid->name)); strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys)); strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));If they are NOT NUL terminated, then keep using
strncpy()but mark the fields in the struct with the__nonstringattribute.
They are supposed to be NUL terminated, but for compatibility reasons we allow them to be not. So I don't think your proposal is safe.
Thanks
David
Quoted 8 lines · David Herrmann
Hi
Quoted 1 line · Kees Cook
On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <[email protected]> wrote:
[...]
Quoted 3 lines
Quoted 2 lines
Can we switch to strscpy instead? This will quiet gcc and avoid the issues with strlcpy.
Quoted 6 lines
Quoted 3 lines
Yes please: it looks like these strings are expected to be NUL terminated, so
strscpy()without the "- 1" andmin()logic would be the correct solution here."the correct solution"? To my knowledge the original code was correct as well. Am I missing something?
So, yes, no one should use strlcpy():
https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
And while I think nothing was technically wrong with the strncpy() usage in the original version, I think strncpy() should only be used for __nonstring cases:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
Quoted 8 lines
Quoted 8 lines
If @hid is already zero, then this would
just be:strscpy(hid->name, ev->u.create2.name, sizeof(hid->name)); strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys)); strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
Quoted 5 lines
Quoted 2 lines
If they are NOT NUL terminated, then keep using
strncpy()but mark the fields in the struct with the__nonstringattribute.They are supposed to be NUL terminated, but for compatibility reasons we allow them to be not. So I don't think your proposal is safe.
I was originally thinking only about the the hid->* strings, so I was confused by this answer (they appear to always be NUL-terminated). Then I thought you meant that ev->u.create2.* strings may not be terminated, but I stayed confused. :)
The original code was:
len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
strncpy(hid->name, ev->u.create2.name, len);If sizeof(hid->name) is smaller than sizeof(ev->u.create2.name), it made sure than hid->name kept a trailing NUL.
If sizeof(ev->u.create2.name) is smaller than sizeof(hid->name), it made sure than the last byte of ev->u.create2.name was ignored, and by definition, hid->name would be NUL-terminated.
So you're converting from a potentially unterminated string into a terminated string... (ev->u.create2.name maybe needs to be marked __nonstring?)
The most you can write is sizeof(dest) - 1 but you must not read more than sizeof(source). So I see that if the destination is smaller than the source, you cannot represent these conditions correctly to strscpy(). (And, I would argue, you can't with strncpy() either.)
However, they're all exactly the same size, so none of this matters, and I think strscpy() would be the most sensible. And maybe you could enforce the size checking:
BUILD_BUG_ON(sizeof(hid->name) != sizeof(ev->u.create2.name));
strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));etc...
Signature · 1 line
Kees Cook
Quoted 33 lines · David Herrmann
This reverts commit
336fd4f5f25157e9e8bd50e898a1bbcd99eaea46.Please note that
strlcpy()does *NOT* do what you think it does.strlcpy()*ALWAYS* reads the full input string, regardless of the 'length' parameter. That is, if the input is not zero-terminated,strlcpy()will *READ* beyond input boundaries. It does this, because it always returns the size it *would* copy if the target was big enough, not the truncated size it actually copied.The original code was perfectly fine. The hid device is zero-initialized and the
strncpy()functions copied up to n-1 characters. The result is always zero-terminated this way.This is the third time someone tried to replace strncpy with strlcpy in this function, and gets it wrong. I now added a comment that should at least make people reconsider.
Signed-off-by David Herrmann <[email protected]>--- drivers/hid/uhid.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)drivers/hid/uhid.c +0
-3@@ -496,12 +496,13 @@ static int uhid_dev_create2(struct uhid_device *uhid, goto err_free; } - len =min (sizeof (hid->name),sizeof (ev->u.create2.name)); -strlcpy (hid->name, ev->u.create2.name, len); - len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys));
drivers/hid/uhid.c static int uhid_dev_create2(struct uhid_device *uhid, · line 505
Quoted 10 lines
drivers/hid/uhid.c +7
-3-strlcpy (hid->phys, ev->u.create2.phys, len); - len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)); -strlcpy (hid->uniq, ev->u.create2.uniq, len); +/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */ + len =min (sizeof (hid->name),sizeof (ev->u.create2.name)) -1 ; +strncpy (hid->name, ev->u.create2.name, len); + len =min (sizeof (hid->phys),sizeof (ev->u.create2.phys)) -1 ; +strncpy (hid->phys, ev->u.create2.phys, len); + len =min (sizeof (hid->uniq),sizeof (ev->u.create2.uniq)) -1 ; +strncpy (hid->uniq, ev->u.create2.uniq, len);
Applied, thanks.
Signature · 2 lines
Jiri Kosina SUSE Labs