LinuxLists.cc - [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

Hi,

On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
> On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > strcpy() performs no bounds checking on the destination buffer. This
> > could result in linear overflows beyond the end of the buffer, leading
> > to all kinds of misbehaviors. The safe replacement is strscpy().
> >
> > Signed-off-by: Len Baker <[email protected]>
> > ---
> > This is a task of the KSPP [1]
> >
> > [1] https://github.com/KSPP/linux/issues/88
> >
> > drivers/input/keyboard/locomokbd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> > index dae053596572..dbb3dc48df12 100644
> > --- a/drivers/input/keyboard/locomokbd.c
> > +++ b/drivers/input/keyboard/locomokbd.c
> > @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
> > locomokbd->suspend_jiffies = jiffies;
> >
> > locomokbd->input = input_dev;
> > - strcpy(locomokbd->phys, "locomokbd/input0");
> > + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
>
> So if the string doesn't fit, it's fine to silently truncate it?

I think it is better than overflow :)

> Rather than converting every single strcpy() in the kernel to
> strscpy(), maybe there should be some consideration given to how the
> issue of a strcpy() that overflows the buffer should be handled.
> E.g. in the case of a known string such as the above, if it's longer
> than the destination, should we find a way to make the compiler issue
> a warning at compile time?

Good point. I am a kernel newbie and have no experience. So this
question should be answered by some kernel hacker :) But I agree
with your proposals.

Kees and folks: Any comments?

Note: Kees is asked the same question in [2]

[2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/

Regards,
Len

2021-08-01 16:46:10

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, 2021-08-01 at 16:00 +0100, Russell King (Oracle) wrote:
> On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > strcpy() performs no bounds checking on the destination buffer. This
> > could result in linear overflows beyond the end of the buffer, leading
> > to all kinds of misbehaviors. The safe replacement is strscpy().
> >
> > Signed-off-by: Len Baker <[email protected]>
> > ---
> > This is a task of the KSPP [1]
> >
> > [1] https://github.com/KSPP/linux/issues/88
> >
> > ?drivers/input/keyboard/locomokbd.c | 2 +-
> > ?1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> > index dae053596572..dbb3dc48df12 100644
> > --- a/drivers/input/keyboard/locomokbd.c
> > +++ b/drivers/input/keyboard/locomokbd.c
> > @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
> > ? locomokbd->suspend_jiffies = jiffies;
> >
> > ? locomokbd->input = input_dev;
> > - strcpy(locomokbd->phys, "locomokbd/input0");
> > + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
>
> So if the string doesn't fit, it's fine to silently truncate it?
>
> Rather than converting every single strcpy() in the kernel to
> strscpy(), maybe there should be some consideration given to how the
> issue of a strcpy() that overflows the buffer should be handled.
> E.g. in the case of a known string such as the above, if it's longer
> than the destination, should we find a way to make the compiler issue
> a warning at compile time?
>

2021-08-01 16:46:59

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
> Hi,
>
> On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
> > On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > > strcpy() performs no bounds checking on the destination buffer. This
> > > could result in linear overflows beyond the end of the buffer, leading
> > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > >
> > > Signed-off-by: Len Baker <[email protected]>
> > > ---
> > > This is a task of the KSPP [1]
> > >
> > > [1] https://github.com/KSPP/linux/issues/88
> > >
> > > drivers/input/keyboard/locomokbd.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> > > index dae053596572..dbb3dc48df12 100644
> > > --- a/drivers/input/keyboard/locomokbd.c
> > > +++ b/drivers/input/keyboard/locomokbd.c
> > > @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
> > > locomokbd->suspend_jiffies = jiffies;
> > >
> > > locomokbd->input = input_dev;
> > > - strcpy(locomokbd->phys, "locomokbd/input0");
> > > + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
> >
> > So if the string doesn't fit, it's fine to silently truncate it?
>
> I think it is better than overflow :)
>
> > Rather than converting every single strcpy() in the kernel to
> > strscpy(), maybe there should be some consideration given to how the
> > issue of a strcpy() that overflows the buffer should be handled.
> > E.g. in the case of a known string such as the above, if it's longer
> > than the destination, should we find a way to make the compiler issue
> > a warning at compile time?
>
> Good point. I am a kernel newbie and have no experience. So this
> question should be answered by some kernel hacker :) But I agree
> with your proposals.
>
> Kees and folks: Any comments?
>
> Note: Kees is asked the same question in [2]
>
> [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/

Hi!

Sorry for the delay at looking into this. It didn't use to be a problem
(there would always have been a compile-time warning generated for
known-too-small cases), but that appears to have regressed when,
ironically, strscpy() coverage was added. I've detailed it in the bug
report:
https://github.com/KSPP/linux/issues/88

So, bottom line: we need to fix the missing compile-time warnings for
strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.

In the past we'd tried to add a stracpy()[1] that would only work with
const string sources. Linus got angry[2] about API explosion, though,
so we're mostly faced with doing the strscpy() replacements.

Another idea might be to have strcpy() do the "constant strings only"
thing, leaving strscpy() for the dynamic lengths.

One thing is clear: replacing strlcpy() with strscpy() is probably the
easiest and best first step to cleaning up the proliferation of str*()
functions.

-Kees

[1] https://lore.kernel.org/lkml/ed4611a4a96057bf8076856560bfbf9b5e95d390.1563889130.git.joe@perches.com/
[2] https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

--
Kees Cook

2021-08-01 16:59:43

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, 2021-08-01 at 09:39 -0700, Joe Perches wrote:
> On Sun, 2021-08-01 at 16:00 +0100, Russell King (Oracle) wrote:
> > On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > > strcpy() performs no bounds checking on the destination buffer. This
> > > could result in linear overflows beyond the end of the buffer, leading
> > > to all kinds of misbehaviors. The safe replacement is strscpy().
[]
> > So if the string doesn't fit, it's fine to silently truncate it?
> >
> > Rather than converting every single strcpy() in the kernel to
> > strscpy(), maybe there should be some consideration given to how the
> > issue of a strcpy() that overflows the buffer should be handled.
> > E.g. in the case of a known string such as the above, if it's longer
> > than the destination, should we find a way to make the compiler issue
> > a warning at compile time?

(apologies for the earlier blank reply, sometimes I dislike my email client)

stracpy could do that with a trivial addition like below:

Old lkml references:

https://lore.kernel.org/lkml/[email protected]/
and
https://lore.kernel.org/lkml/[email protected]/

But Linus T wants a copy_string mechanism instead:
https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

/**
* stracpy - Copy a C-string into an array of char/u8/s8 or equivalent
* @dest: Where to copy the string, must be an array of char and not a pointer
* @src: String to copy, may be a pointer or const char array
*
* Helper for strscpy().
* Copies a maximum of sizeof(@dest) bytes of @src with %NUL termination.
*
* A BUILD_BUG_ON is used for cases where @dest is not a char array or
* @src is a char array and is larger than @dest.
*
* Returns:
* * The number of characters copied (not including the trailing %NUL)
* * -E2BIG if @dest is a zero size array or @src was truncated.
*/
#define stracpy(dest, src) \
({ \
BUILD_BUG_ON(!(__same_type(dest, char[]) || \
__same_type(dest, unsigned char[]) || \
__same_type(dest, signed char[]))); \
BUILD_BUG_ON((__same_type(src, char[]) || \
__same_type(src, unsigned char[]) || \
__same_type(src, signed char[])) && \
ARRAY_SIZE(src) > ARRAY_SIZE(dest)); \
\
strscpy(dest, src, ARRAY_SIZE(dest)); \
})

2021-08-01 17:20:58

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
> On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
> > Hi,
> >
> > On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
> > > On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > > > strcpy() performs no bounds checking on the destination buffer. This
> > > > could result in linear overflows beyond the end of the buffer, leading
> > > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > > >
> > > > Signed-off-by: Len Baker <[email protected]>
> > > > ---
> > > > This is a task of the KSPP [1]
> > > >
> > > > [1] https://github.com/KSPP/linux/issues/88
> > > >
> > > > drivers/input/keyboard/locomokbd.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> > > > index dae053596572..dbb3dc48df12 100644
> > > > --- a/drivers/input/keyboard/locomokbd.c
> > > > +++ b/drivers/input/keyboard/locomokbd.c
> > > > @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
> > > > locomokbd->suspend_jiffies = jiffies;
> > > >
> > > > locomokbd->input = input_dev;
> > > > - strcpy(locomokbd->phys, "locomokbd/input0");
> > > > + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
> > >
> > > So if the string doesn't fit, it's fine to silently truncate it?
> >
> > I think it is better than overflow :)
> >
> > > Rather than converting every single strcpy() in the kernel to
> > > strscpy(), maybe there should be some consideration given to how the
> > > issue of a strcpy() that overflows the buffer should be handled.
> > > E.g. in the case of a known string such as the above, if it's longer
> > > than the destination, should we find a way to make the compiler issue
> > > a warning at compile time?
> >
> > Good point. I am a kernel newbie and have no experience. So this
> > question should be answered by some kernel hacker :) But I agree
> > with your proposals.
> >
> > Kees and folks: Any comments?
> >
> > Note: Kees is asked the same question in [2]
> >
> > [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/
>
> Hi!
>
> Sorry for the delay at looking into this. It didn't use to be a problem
> (there would always have been a compile-time warning generated for
> known-too-small cases), but that appears to have regressed when,
> ironically, strscpy() coverage was added. I've detailed it in the bug
> report:
> https://github.com/KSPP/linux/issues/88
>
> So, bottom line: we need to fix the missing compile-time warnings for
> strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.

Is it possible to have them warn always? Or that would be too many false
positives?

>
> In the past we'd tried to add a stracpy()[1] that would only work with
> const string sources. Linus got angry[2] about API explosion, though,
> so we're mostly faced with doing the strscpy() replacements.

I would like to have an API that would do compile-time checks and
BUILD_BUG_ON() for a few places in input drivers where we copy constant
strings. There is no reason to encumber the code with runtime checks,
and bombing out on compile instead of truncating would be nice.

>
> Another idea might be to have strcpy() do the "constant strings only"
> thing, leaving strscpy() for the dynamic lengths.
>
> One thing is clear: replacing strlcpy() with strscpy() is probably the
> easiest and best first step to cleaning up the proliferation of str*()
> functions.

OK, so the consensus is that we set this patch aside as it does not
really fix any issues (the strcpy() destination is 32 bytes and is big
enough to hold the string being copied)?

Thanks.

--
Dmitry

2021-08-02 16:14:54

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, Aug 01, 2021 at 09:55:28AM -0700, Joe Perches wrote:
> On Sun, 2021-08-01 at 09:39 -0700, Joe Perches wrote:
> > On Sun, 2021-08-01 at 16:00 +0100, Russell King (Oracle) wrote:
> > > On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > > > strcpy() performs no bounds checking on the destination buffer. This
> > > > could result in linear overflows beyond the end of the buffer, leading
> > > > to all kinds of misbehaviors. The safe replacement is strscpy().
> []
> > > So if the string doesn't fit, it's fine to silently truncate it?
> > >
> > > Rather than converting every single strcpy() in the kernel to
> > > strscpy(), maybe there should be some consideration given to how the
> > > issue of a strcpy() that overflows the buffer should be handled.
> > > E.g. in the case of a known string such as the above, if it's longer
> > > than the destination, should we find a way to make the compiler issue
> > > a warning at compile time?
>
> (apologies for the earlier blank reply, sometimes I dislike my email client)
>
> stracpy could do that with a trivial addition like below:
>
> Old lkml references:
>
> https://lore.kernel.org/lkml/[email protected]/
> and
> https://lore.kernel.org/lkml/[email protected]/
>
> But Linus T wants a copy_string mechanism instead:
> https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
>
> /**
> * stracpy - Copy a C-string into an array of char/u8/s8 or equivalent
> * @dest: Where to copy the string, must be an array of char and not a pointer
> * @src: String to copy, may be a pointer or const char array
> *
> * Helper for strscpy().
> * Copies a maximum of sizeof(@dest) bytes of @src with %NUL termination.
> *
> * A BUILD_BUG_ON is used for cases where @dest is not a char array or
> * @src is a char array and is larger than @dest.
> *
> * Returns:
> * * The number of characters copied (not including the trailing %NUL)
> * * -E2BIG if @dest is a zero size array or @src was truncated.
> */
> #define stracpy(dest, src) \
> ({ \
> BUILD_BUG_ON(!(__same_type(dest, char[]) || \
> __same_type(dest, unsigned char[]) || \
> __same_type(dest, signed char[]))); \
> BUILD_BUG_ON((__same_type(src, char[]) || \
> __same_type(src, unsigned char[]) || \
> __same_type(src, signed char[])) && \
> ARRAY_SIZE(src) > ARRAY_SIZE(dest)); \
> \
> strscpy(dest, src, ARRAY_SIZE(dest)); \
> })

I'm wondering, instead, if we could convert strcpy() into this instead
of adding another API? I.e. convert all the places that warn (if this
were strcpy), and then land the conversion.

--
Kees Cook

2021-08-02 16:19:16

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, Aug 01, 2021 at 10:19:07AM -0700, Dmitry Torokhov wrote:
> On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
> > On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
> > > Hi,
> > >
> > > On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
> > > > On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > > > > strcpy() performs no bounds checking on the destination buffer. This
> > > > > could result in linear overflows beyond the end of the buffer, leading
> > > > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > > > >
> > > > > Signed-off-by: Len Baker <[email protected]>
> > > > > ---
> > > > > This is a task of the KSPP [1]
> > > > >
> > > > > [1] https://github.com/KSPP/linux/issues/88
> > > > >
> > > > > drivers/input/keyboard/locomokbd.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> > > > > index dae053596572..dbb3dc48df12 100644
> > > > > --- a/drivers/input/keyboard/locomokbd.c
> > > > > +++ b/drivers/input/keyboard/locomokbd.c
> > > > > @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
> > > > > locomokbd->suspend_jiffies = jiffies;
> > > > >
> > > > > locomokbd->input = input_dev;
> > > > > - strcpy(locomokbd->phys, "locomokbd/input0");
> > > > > + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
> > > >
> > > > So if the string doesn't fit, it's fine to silently truncate it?
> > >
> > > I think it is better than overflow :)
> > >
> > > > Rather than converting every single strcpy() in the kernel to
> > > > strscpy(), maybe there should be some consideration given to how the
> > > > issue of a strcpy() that overflows the buffer should be handled.
> > > > E.g. in the case of a known string such as the above, if it's longer
> > > > than the destination, should we find a way to make the compiler issue
> > > > a warning at compile time?
> > >
> > > Good point. I am a kernel newbie and have no experience. So this
> > > question should be answered by some kernel hacker :) But I agree
> > > with your proposals.
> > >
> > > Kees and folks: Any comments?
> > >
> > > Note: Kees is asked the same question in [2]
> > >
> > > [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/
> >
> > Hi!
> >
> > Sorry for the delay at looking into this. It didn't use to be a problem
> > (there would always have been a compile-time warning generated for
> > known-too-small cases), but that appears to have regressed when,
> > ironically, strscpy() coverage was added. I've detailed it in the bug
> > report:
> > https://github.com/KSPP/linux/issues/88
> >
> > So, bottom line: we need to fix the missing compile-time warnings for
> > strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.
>
> Is it possible to have them warn always? Or that would be too many false
> positives?

There are actually no false positives right now (they were all fixed
before FORTIFY_SOURCE landed). Enabling it by default would likely mean
splitting compile-time checks from run-time checks... I'm kind of
already doing this in the recent memcpy() strictness series[1], so ...
maybe?

I think I'd like to land the memcpy() series first, then we can revisit
making it always warn.

> > In the past we'd tried to add a stracpy()[1] that would only work with
> > const string sources. Linus got angry[2] about API explosion, though,
> > so we're mostly faced with doing the strscpy() replacements.
>
> I would like to have an API that would do compile-time checks and
> BUILD_BUG_ON() for a few places in input drivers where we copy constant
> strings. There is no reason to encumber the code with runtime checks,
> and bombing out on compile instead of truncating would be nice.

In theory, this is provided by CONFIG_FORTIFY_SOURCE, though a recent
change broke a specific instance. I've added tests for this now to the
memcpy() series, and will get it fixed in there too.

> > Another idea might be to have strcpy() do the "constant strings only"
> > thing, leaving strscpy() for the dynamic lengths.
> >
> > One thing is clear: replacing strlcpy() with strscpy() is probably the
> > easiest and best first step to cleaning up the proliferation of str*()
> > functions.
>
> OK, so the consensus is that we set this patch aside as it does not
> really fix any issues (the strcpy() destination is 32 bytes and is big
> enough to hold the string being copied)?

I think that's fair.

-Kees

[1] https://lore.kernel.org/lkml/[email protected]/

--
Kees Cook

2021-08-02 18:58:44

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Mon, 2021-08-02 at 09:13 -0700, Kees Cook wrote:
> I'm wondering, instead, if we could convert strcpy() into this instead
> of adding another API? I.e. convert all the places that warn (if this
> were strcpy), and then land the conversion.

Perhaps not as strcpy is a builtin.

It might be easier as a cocci script. Something like:

@@
char [] dest;
constant char [] src;
@@

* strcpy(dest, src)

There are some additional test that needs to be added so that
only length(src) > length(dest) is reported.

2021-08-03 07:09:40

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
> On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
> > Hi,
> >
> > On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
> > > On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
> > > > strcpy() performs no bounds checking on the destination buffer. This
> > > > could result in linear overflows beyond the end of the buffer, leading
> > > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > > >
> > > > Signed-off-by: Len Baker <[email protected]>
> > > > ---
> > > > This is a task of the KSPP [1]
> > > >
> > > > [1] https://github.com/KSPP/linux/issues/88
> > > >
> > > > drivers/input/keyboard/locomokbd.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> > > > index dae053596572..dbb3dc48df12 100644
> > > > --- a/drivers/input/keyboard/locomokbd.c
> > > > +++ b/drivers/input/keyboard/locomokbd.c
> > > > @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
> > > > locomokbd->suspend_jiffies = jiffies;
> > > >
> > > > locomokbd->input = input_dev;
> > > > - strcpy(locomokbd->phys, "locomokbd/input0");
> > > > + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
> > >
> > > So if the string doesn't fit, it's fine to silently truncate it?
> >
> > I think it is better than overflow :)
> >
> > > Rather than converting every single strcpy() in the kernel to
> > > strscpy(), maybe there should be some consideration given to how the
> > > issue of a strcpy() that overflows the buffer should be handled.
> > > E.g. in the case of a known string such as the above, if it's longer
> > > than the destination, should we find a way to make the compiler issue
> > > a warning at compile time?
> >
> > Good point. I am a kernel newbie and have no experience. So this
> > question should be answered by some kernel hacker :) But I agree
> > with your proposals.
> >
> > Kees and folks: Any comments?
> >
> > Note: Kees is asked the same question in [2]
> >
> > [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/
>
> Hi!
>
> Sorry for the delay at looking into this. It didn't use to be a problem
> (there would always have been a compile-time warning generated for
> known-too-small cases), but that appears to have regressed when,
> ironically, strscpy() coverage was added. I've detailed it in the bug
> report:
> https://github.com/KSPP/linux/issues/88
>
> So, bottom line: we need to fix the missing compile-time warnings for
> strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.

I've got these fixed now, and will send them out likely tomorrow, but I
did, in fact, find 4 cases of truncation, all in v4l, and all appear to
have been truncated since introduction:

struct v4l2_capability {
...
__u8 card[32];
(stores 31 characters)

drivers/media/radio/radio-wl1273.c:1282
wl1273_fm_vidioc_querycap()
strscpy(capability->card, "Texas Instruments Wl1273 FM Radio",
sizeof(capability->card));
33 characters, getting truncated to:
Texas Instruments Wl1273 FM Rad
87d1a50ce45168cbaec10397e876286a398052c1

drivers/media/radio/si470x/radio-si470x-usb.c:514
si470x_vidioc_querycap()
#define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
strscpy(capability->card, DRIVER_CARD,
sizeof(capability->card));
37 characters, getting truncated to:
Silicon Labs Si470x FM Radio Re
78656acdcf4852547a29e929a1b7a98d5ac65f17

drivers/media/radio/si470x/radio-si470x-i2c.c:225
si470x_vidioc_querycap()
#define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
strscpy(capability->card, DRIVER_CARD,
sizeof(capability->card));
37 characters, getting truncated to:
Silicon Labs Si470x FM Radio Re
cc35bbddfe10f77d949f0190764b252cd2b70c3c

drivers/media/usb/tm6000/tm6000-video.c:855
vidioc_querycap()
strscpy(cap->card, "Trident TVMaster TM5600/6000/6010",
sizeof(cap->card));
33 characters, getting truncated to:
Trident TVMaster TM5600/6000/60
e28f49b0b2a8e678af62745ffdc4e4f36d7283a6

How should these be handled? I assume v4l2_capability::card can't be
resized since it's part of IOCTL response, so likely all the string just
need to be shortened in some way? Seems like dropping the manufacturer
name makes the most sense, since manufacturer can be kind of derived
from the driver names.

Thoughts?

-Kees

--
Kees Cook

2021-08-03 07:24:04

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On 03/08/2021 09:07, Kees Cook wrote:
> On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
>> On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
>>> Hi,
>>>
>>> On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
>>>> On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
>>>>> strcpy() performs no bounds checking on the destination buffer. This
>>>>> could result in linear overflows beyond the end of the buffer, leading
>>>>> to all kinds of misbehaviors. The safe replacement is strscpy().
>>>>>
>>>>> Signed-off-by: Len Baker <[email protected]>
>>>>> ---
>>>>> This is a task of the KSPP [1]
>>>>>
>>>>> [1] https://github.com/KSPP/linux/issues/88
>>>>>
>>>>> drivers/input/keyboard/locomokbd.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
>>>>> index dae053596572..dbb3dc48df12 100644
>>>>> --- a/drivers/input/keyboard/locomokbd.c
>>>>> +++ b/drivers/input/keyboard/locomokbd.c
>>>>> @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
>>>>> locomokbd->suspend_jiffies = jiffies;
>>>>>
>>>>> locomokbd->input = input_dev;
>>>>> - strcpy(locomokbd->phys, "locomokbd/input0");
>>>>> + strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
>>>>
>>>> So if the string doesn't fit, it's fine to silently truncate it?
>>>
>>> I think it is better than overflow :)
>>>
>>>> Rather than converting every single strcpy() in the kernel to
>>>> strscpy(), maybe there should be some consideration given to how the
>>>> issue of a strcpy() that overflows the buffer should be handled.
>>>> E.g. in the case of a known string such as the above, if it's longer
>>>> than the destination, should we find a way to make the compiler issue
>>>> a warning at compile time?
>>>
>>> Good point. I am a kernel newbie and have no experience. So this
>>> question should be answered by some kernel hacker :) But I agree
>>> with your proposals.
>>>
>>> Kees and folks: Any comments?
>>>
>>> Note: Kees is asked the same question in [2]
>>>
>>> [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/
>>
>> Hi!
>>
>> Sorry for the delay at looking into this. It didn't use to be a problem
>> (there would always have been a compile-time warning generated for
>> known-too-small cases), but that appears to have regressed when,
>> ironically, strscpy() coverage was added. I've detailed it in the bug
>> report:
>> https://github.com/KSPP/linux/issues/88
>>
>> So, bottom line: we need to fix the missing compile-time warnings for
>> strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.
>
> I've got these fixed now, and will send them out likely tomorrow, but I
> did, in fact, find 4 cases of truncation, all in v4l, and all appear to
> have been truncated since introduction:
>
> struct v4l2_capability {
> ...
> __u8 card[32];
> (stores 31 characters)
>
> drivers/media/radio/radio-wl1273.c:1282
> wl1273_fm_vidioc_querycap()
> strscpy(capability->card, "Texas Instruments Wl1273 FM Radio",
> sizeof(capability->card));
> 33 characters, getting truncated to:
> Texas Instruments Wl1273 FM Rad
> 87d1a50ce45168cbaec10397e876286a398052c1

I'd change this to: "TI WL1273 FM Radio"

>
> drivers/media/radio/si470x/radio-si470x-usb.c:514
> si470x_vidioc_querycap()
> #define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
> strscpy(capability->card, DRIVER_CARD,
> sizeof(capability->card));
> 37 characters, getting truncated to:
> Silicon Labs Si470x FM Radio Re
> 78656acdcf4852547a29e929a1b7a98d5ac65f17

This to "Silicon Labs Si470x FM Radio"

>
> drivers/media/radio/si470x/radio-si470x-i2c.c:225
> si470x_vidioc_querycap()
> #define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
> strscpy(capability->card, DRIVER_CARD,
> sizeof(capability->card));
> 37 characters, getting truncated to:
> Silicon Labs Si470x FM Radio Re
> cc35bbddfe10f77d949f0190764b252cd2b70c3c

Ditto.

>
> drivers/media/usb/tm6000/tm6000-video.c:855
> vidioc_querycap()
> strscpy(cap->card, "Trident TVMaster TM5600/6000/6010",
> sizeof(cap->card));
> 33 characters, getting truncated to:
> Trident TVMaster TM5600/6000/60
> e28f49b0b2a8e678af62745ffdc4e4f36d7283a6

And this to: "Trident TM5600/6000/6010"

The truncation doesn't hurt anything, it's just looks a bit ugly.
These shorter names should solve this issue.

Regards,

Hans

>
> How should these be handled? I assume v4l2_capability::card can't be
> resized since it's part of IOCTL response, so likely all the string just
> need to be shortened in some way? Seems like dropping the manufacturer
> name makes the most sense, since manufacturer can be kind of derived
> from the driver names.
>
> Thoughts?
>
> -Kees
>

2021-08-07 14:04:18

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

Hi,

On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
> On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
> > Hi,
> >
> > On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
> > > Rather than converting every single strcpy() in the kernel to
> > > strscpy(), maybe there should be some consideration given to how the
> > > issue of a strcpy() that overflows the buffer should be handled.
> > > E.g. in the case of a known string such as the above, if it's longer
> > > than the destination, should we find a way to make the compiler issue
> > > a warning at compile time?
> >
> > Good point. I am a kernel newbie and have no experience. So this
> > question should be answered by some kernel hacker :) But I agree
> > with your proposals.
> >
> > Kees and folks: Any comments?
> >
> > Note: Kees is asked the same question in [2]
> >
> > [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/
>
> Hi!
>
> Sorry for the delay at looking into this. It didn't use to be a problem
> (there would always have been a compile-time warning generated for
> known-too-small cases), but that appears to have regressed when,
> ironically, strscpy() coverage was added. I've detailed it in the bug
> report:
> https://github.com/KSPP/linux/issues/88
>
> So, bottom line: we need to fix the missing compile-time warnings for
> strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.
>
> In the past we'd tried to add a stracpy()[1] that would only work with
> const string sources. Linus got angry[2] about API explosion, though,
> so we're mostly faced with doing the strscpy() replacements.
>
> Another idea might be to have strcpy() do the "constant strings only"
> thing, leaving strscpy() for the dynamic lengths.
>
> One thing is clear: replacing strlcpy() with strscpy() is probably the
> easiest and best first step to cleaning up the proliferation of str*()
> functions.

Thanks for all this info. I will work on it (clean up the proliferation
of str*() functions).

Regards,
Len

>
> -Kees
>
> [1] https://lore.kernel.org/lkml/ed4611a4a96057bf8076856560bfbf9b5e95d390.1563889130.git.joe@perches.com/
> [2] https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
>
> --
> Kees Cook

2021-08-07 14:11:18

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

Hi Joe,

On Mon, Aug 02, 2021 at 11:57:40AM -0700, Joe Perches wrote:
> On Mon, 2021-08-02 at 09:13 -0700, Kees Cook wrote:
> > I'm wondering, instead, if we could convert strcpy() into this instead
> > of adding another API? I.e. convert all the places that warn (if this
> > were strcpy), and then land the conversion.
>
> Perhaps not as strcpy is a builtin.
>
> It might be easier as a cocci script. Something like:
>
> @@
> char [] dest;
> constant char [] src;
> @@
>
> * strcpy(dest, src)
>
> There are some additional test that needs to be added so that
> only length(src) > length(dest) is reported.
>
Thanks for the ideas. I will think on this.

Regards,
Len

2021-08-07 15:19:00

[permalink] [raw]

Subject: Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

On Sat, 2021-08-07 at 16:02 +0200, Len Baker wrote:
> On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
[]
> > One thing is clear: replacing strlcpy() with strscpy() is probably the
> > easiest and best first step to cleaning up the proliferation of str*()
> > functions.
>
> Thanks for all this info. I will work on it (clean up the proliferation
> of str*() functions).

btw:

It's not possible to sed as the return value is different,
but here is a cocci script that converts strlcpy to strscpy
when the return value is unused.

@@
expression e1, e2, e3;
@@

- strlcpy(
+ strscpy(
e1, e2, e3);

This cocci script was used on sound/ awhile back.
see commit 75b1a8f9d62e.

2021-08-08 11:43:53