| Message ID | 20181114131642.21425-1-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Revert "HID: uhid: use strlcpy() instead of strncpy()" | expand |
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; >
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; >> >
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
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...
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.
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;
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(-)