netdev - Re: [PATCH v8 1/8] Get rid of __get_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <hxwogrharokumnbphayi22qnl4yxeqxkxdddjkshso3nztroq5@lb3wbcsp3vhf>
Date: Wed, 28 Aug 2024 16:10:49 +0200
From: Alejandro Colomar <alx@...nel.org>
To: Yafang Shao <laoar.shao@...il.com>
Cc: akpm@...ux-foundation.org, torvalds@...ux-foundation.org, 
	justinstitt@...gle.com, ebiederm@...ssion.com, alexei.starovoitov@...il.com, 
	rostedt@...dmis.org, catalin.marinas@....com, penguin-kernel@...ove.sakura.ne.jp, 
	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, audit@...r.kernel.org, linux-security-module@...r.kernel.org, 
	selinux@...r.kernel.org, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Kees Cook <keescook@...omium.org>, 
	Matus Jokay <matus.jokay@...ba.sk>, "Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH v8 1/8] Get rid of __get_task_comm()

Hi Yafang,

On Wed, Aug 28, 2024 at 09:40:35PM GMT, Yafang Shao wrote:
> > Ahh, the actual generic definition is in <include/linux/string.h>.
> > You could do
> >
> >         diff --git i/include/linux/string.h w/include/linux/string.h
> >         index 9edace076ddb..060504719904 100644
> >         --- i/include/linux/string.h
> >         +++ w/include/linux/string.h
> >         @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
> >           * known size.
> >           */
> >          #define __strscpy0(dst, src, ...)      \
> >         -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> >         +       sized_strscpy(dst, src, ARRAY_SIZE(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))
> >         +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
> >          #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
> >
> >          /**
> 
> Thank you for your suggestion. How does the following commit log look
> to you? Does it meet your expectations?
> 
>     string: Use ARRAY_SIZE() in strscpy()
> 
>     We can use ARRAY_SIZE() instead to clarify that they are regular characters.

I would write the following:

For symmetry with wide-character string functions, ARRAY_SIZE() is more
appropriate than sizeof().

For example, one would call wcs*cpy(dst, src, ARRAY_SIZE(dst)).
The call wcs*cpy(dst, src, sizeof(dst)) would be bogus, since the
argument is the number of wide characters, not the number of bytes.

When translating that to normal characters, one wants conceptually the
same operation, but on (normal) characters.  That is, one wants
strscpy(dst, src, ARRAY_SIZE(dst)).  While strscpy() with sizeof() works
just fine because sizeof(char)==1 by definition, it is conceptually
wrong to use it.

By using ARRAY_SIZE(), we get the __must_be_array() check for free.

> 
>     Co-developed-by: Alejandro Colomar <alx@...nel.org>
>     Signed-off-by: Alejandro Colomar <alx@...nel.org>
>     Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> 
> diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
> index bbab79c0c074..07216996e3a9 100644
> --- a/arch/um/include/shared/user.h
> +++ b/arch/um/include/shared/user.h
> @@ -14,7 +14,7 @@
>   * copying too much infrastructure for my taste, so userspace files
>   * get less checking than kernel files.
>   */
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
> 
>  /* This is to get size_t and NULL */
>  #ifndef __UM_HOST__
> @@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
> const char *prefix_str,
>  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)      sized_strscpy(dst, src, sizeof(dst))
> +#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))
> 
>  /* Copied from linux/compiler-gcc.h since we can't include it directly */
>  #define barrier() __asm__ __volatile__("": : :"memory")
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 9edace076ddb..060504719904 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> 
> @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>   * known size.
>   */
>  #define __strscpy0(dst, src, ...)      \
> -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> +       sized_strscpy(dst, src, ARRAY_SIZE(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))
> +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
>  #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)

The diff looks good to me.  Thanks!

Cheers,
Alex

> 
>  /**
> 
> --
> Regards
> 
> Yafang

-- 
<https://www.alejandro-colomar.es/>

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)