Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

[Geert Uytterhoeven]

Hi Kees,

On Mon, Feb 5, 2024 at 1:37 PM Kees Cook <keescook@chromium.org> wrote:
> Using sizeof(dst) for the "size" argument in strscpy() is the
> overwhelmingly common case. Instead of requiring this everywhere, allow a
> 2-argument version to be used that will use the sizeof() internally. There
> are other functions in the kernel with optional arguments[1], so this
> isn't unprecedented, and improves readability. Update and relocate the
> kern-doc for strscpy() too.
>
> Adjust ARCH=um build to notice the changed export name, as it doesn't
> do full header includes for the string helpers.
>
> This could additionally let us save a few hundred lines of code:
>  1177 files changed, 2455 insertions(+), 3026 deletions(-)
> with a treewide cleanup using Coccinelle:
>
> @needless_arg@
> expression DST, SRC;
> @@
>
>         strscpy(DST, SRC
> -, sizeof(DST)
>         )
>
> Link: https://elixir.bootlin.com/linux/v6.7/source/include/linux/pci.h#L1517 [1]
> Reviewed-by: Justin Stitt <justinstitt@google.com>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Thanks for your patch!

> --- a/include/linux/string.h
> +++ b/include/linux/string.h

> +/*
> + * The 2 argument style can only be used when dst is an array with a
> + * known size.
> + */
> +#define __strscpy0(dst, src, ...)      \
> +       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> +#define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)

(dst), (src), (size) etc.


> +
> +/**
> + * strscpy - Copy a C-string into a sized buffer
> + * @dst: Where to copy the string to
> + * @src: Where to copy the string from
> + * @...: Size of destination buffer (optional)
> + *
> + * Copy the source string @src, or as much of it as fits, into the
> + * destination @dst buffer. The behavior is undefined if the string
> + * buffers overlap. The destination @dst buffer is always NUL terminated,
> + * unless it's zero-sized.
> + *
> + * The size argument @... is only required when @dst is not an array, or
> + * when the copy needs to be smaller than sizeof(@dst).
> + *
> + * Preferred to strncpy() since it always returns a valid string, and
> + * doesn't unnecessarily force the tail of the destination buffer to be
> + * zero padded. If padding is desired please use strscpy_pad().
> + *
> + * Returns the number of characters copied in @dst (not including the
> + * trailing %NUL) or -E2BIG if @size is 0 or the copy from @src was
> + * truncated.
> + */
> +#define strscpy(dst, src, ...) \
> +       CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)

Likewise

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

[Andy Shevchenko]

On Mon, Feb 05, 2024 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 5, 2024 at 1:37 PM Kees Cook <keescook@chromium.org> wrote:

...

> > +#define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
> 
> (dst), (src), (size) etc.

No need.

...

> > +#define strscpy(dst, src, ...) \
> > +       CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
> 
> Likewise

Likewise

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

[Geert Uytterhoeven]

Hi Kees,

On Mon, Feb 5, 2024 at 2:01 PM Kees Cook <keescook@chromium.org> wrote:
> On Mon, Feb 05, 2024 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> > > +/*
> > > + * The 2 argument style can only be used when dst is an array with a
> > > + * known size.
> > > + */
> > > +#define __strscpy0(dst, src, ...)      \
> > > +       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> > > +#define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
> >
> > (dst), (src), (size) etc.
>
> I normally don't do this when macro args are being expanded into
> function arguments. I've only done it for when macro args are used in
> expressions. Am I missing a side-effect here, or is this more about
> stylistic consistency?

I'm not 100% sure it is needed, but I'm always wary when using macro
parameters without parentheses, except in the most simple use-cases.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] string: Allow 2-argument strscpy_pad()

[Geert Uytterhoeven]

Hi Kees,

On Mon, Feb 5, 2024 at 1:36 PM Kees Cook <keescook@chromium.org> wrote:
> Similar to strscpy(), update strscpy_pad()'s 3rd argument to be
> optional when the destination is a compile-time known size array.
>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Thanks for your patch!

> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -78,6 +78,10 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>         sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
>  #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
>
> +#define __strscpy_pad0(dst, src, ...)  \
> +       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
> +#define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)

(dst) etc.

> @@ -123,17 +139,8 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>   * * The number of characters copied (not including the trailing %NULs)
>   * * -E2BIG if count is 0 or @src was truncated.
>   */
> -#define strscpy_pad(dest, src, count)  ({                      \
> -       char *__dst = (dest);                                           \
> -       const char *__src = (src);                                      \
> -       const size_t __count = (count);                                 \
> -       ssize_t __wrote;                                                \
> -                                                                       \
> -       __wrote = strscpy(__dst, __src, __count);                       \
> -       if (__wrote >= 0 && __wrote < __count)                          \
> -               memset(__dst + __wrote + 1, 0, __count - __wrote - 1);  \
> -       __wrote;                                                        \
> -})
> +#define strscpy_pad(dst, src, ...)     \
> +       CONCATENATE(__strscpy_pad, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)

Likewise,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] string: Allow 2-argument strscpy_pad()

[Andy Shevchenko]

On Mon, Feb 05, 2024 at 01:48:51PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 5, 2024 at 1:36 PM Kees Cook <keescook@chromium.org> wrote:

...

> > +#define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
> 
> (dst) etc.

Makes a little sense here. Are you expecting, e.g., dst to be 'a, b' (w/o
quotes where a and b are expressions)?

...

> > +#define strscpy_pad(dst, src, ...)     \
> > +       CONCATENATE(__strscpy_pad, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
> 
> Likewise,

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] um: Convert strscpy() usage to 2-argument style

[Geert Uytterhoeven]

Hi Kees,

On Mon, Feb 5, 2024 at 1:36 PM Kees Cook <keescook@chromium.org> wrote:
> The ARCH=um build has its own idea about strscpy()'s definition. Adjust
> the callers to remove the redundant sizeof() arguments ahead of treewide
> changes, since it needs a manual adjustment for the newly named
> sized_strscpy() export.
>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: linux-um@lists.infradead.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Thanks for your patch!

> --- a/arch/um/include/shared/user.h
> +++ b/arch/um/include/shared/user.h
> @@ -52,7 +52,7 @@ static inline int printk(const char *fmt, ...)
>  extern int in_aton(char *str);
>  extern size_t strlcat(char *, const char *, size_t);
>  extern size_t sized_strscpy(char *, const char *, size_t);
> -#define strscpy(dst, src, size)        sized_strscpy(dst, src, size)
> +#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))

(dst), (src)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/4] um: Convert strscpy() usage to 2-argument style

[Andy Shevchenko]

On Mon, Feb 05, 2024 at 01:50:14PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 5, 2024 at 1:36 PM Kees Cook <keescook@chromium.org> wrote:

...

> > +#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
> 
> (dst), (src)

No need.

-- 
With Best Regards,
Andy Shevchenko