* [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" @ 2018-11-14 13:16 David Herrmann 2018-11-14 15:40 ` Laura Abbott 2018-11-19 13:33 ` Jiri Kosina 0 siblings, 2 replies; 6+ messages in thread From: David Herrmann @ 2018-11-14 13:16 UTC (permalink / raw) To: linux-input; +Cc: linux-kernel, jikos, benjamin.tissoires, 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 <dh.herrmann@gmail.com> --- drivers/hid/uhid.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index fefedc0b4dc6..0dfdd0ac7120 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -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; -- 2.19.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" 2018-11-14 13:16 [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" David Herrmann @ 2018-11-14 15:40 ` Laura Abbott 2018-11-14 23:09 ` Kees Cook 2018-11-19 13:33 ` Jiri Kosina 1 sibling, 1 reply; 6+ messages in thread From: Laura Abbott @ 2018-11-14 15:40 UTC (permalink / raw) To: David Herrmann, linux-input Cc: linux-kernel, jikos, benjamin.tissoires, Kees Cook On 11/14/18 5:16 AM, David Herrmann wrote: > 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. > Can we switch to strscpy instead? This will quiet gcc and avoid the issues with strlcpy. > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/hid/uhid.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c > index fefedc0b4dc6..0dfdd0ac7120 100644 > --- a/drivers/hid/uhid.c > +++ b/drivers/hid/uhid.c > @@ -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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" 2018-11-14 15:40 ` Laura Abbott @ 2018-11-14 23:09 ` Kees Cook 2018-11-15 11:55 ` David Herrmann 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2018-11-14 23:09 UTC (permalink / raw) To: David Herrmann Cc: Laura Abbott, linux-input, LKML, Jiri Kosina, Benjamin Tissoires, Xiongfeng Wang On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <labbott@redhat.com> wrote: > On 11/14/18 5:16 AM, David Herrmann wrote: >> >> 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. >> > > 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 > > >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> --- >> drivers/hid/uhid.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c >> index fefedc0b4dc6..0dfdd0ac7120 100644 >> --- a/drivers/hid/uhid.c >> +++ b/drivers/hid/uhid.c >> @@ -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; >> > -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" 2018-11-14 23:09 ` Kees Cook @ 2018-11-15 11:55 ` David Herrmann 2018-11-16 1:10 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: David Herrmann @ 2018-11-15 11:55 UTC (permalink / raw) To: Kees Cook Cc: labbott, open list:HID CORE LAYER, linux-kernel, Jiri Kosina, Benjamin Tissoires, xiongfeng.wang Hi On Thu, Nov 15, 2018 at 12:09 AM Kees Cook <keescook@chromium.org> wrote: > On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <labbott@redhat.com> wrote: [...] > > 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. "the correct solution"? To my knowledge the original code was correct as well. Am I missing something? > 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. 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" 2018-11-15 11:55 ` David Herrmann @ 2018-11-16 1:10 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2018-11-16 1:10 UTC (permalink / raw) To: David Herrmann Cc: Laura Abbott, open list:HID CORE LAYER, linux-kernel, Jiri Kosina, Benjamin Tissoires, Xiongfeng Wang On Thu, Nov 15, 2018 at 5:55 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Thu, Nov 15, 2018 at 12:09 AM Kees Cook <keescook@chromium.org> wrote: >> On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <labbott@redhat.com> wrote: > [...] >> > 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. > > "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 > >> 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. > > 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... -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" 2018-11-14 13:16 [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" David Herrmann 2018-11-14 15:40 ` Laura Abbott @ 2018-11-19 13:33 ` Jiri Kosina 1 sibling, 0 replies; 6+ messages in thread From: Jiri Kosina @ 2018-11-19 13:33 UTC (permalink / raw) To: David Herrmann; +Cc: linux-input, linux-kernel, benjamin.tissoires On Wed, 14 Nov 2018, David Herrmann wrote: > 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 <dh.herrmann@gmail.com> > --- > drivers/hid/uhid.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c > index fefedc0b4dc6..0dfdd0ac7120 100644 > --- a/drivers/hid/uhid.c > +++ b/drivers/hid/uhid.c > @@ -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); Applied, thanks. -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-19 13:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-14 13:16 [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()" David Herrmann 2018-11-14 15:40 ` Laura Abbott 2018-11-14 23:09 ` Kees Cook 2018-11-15 11:55 ` David Herrmann 2018-11-16 1:10 ` Kees Cook 2018-11-19 13:33 ` Jiri Kosina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).