[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <q6xvpwqj7dkgu2cay5mgahscfgdwu2ohzxs7xd3nw3xa622sh4@u35topnxx36b>
Date: Thu, 29 Aug 2024 02:25:00 +0200
From: Alejandro Colomar <alx@...nel.org>
To: Kees Cook <kees@...nel.org>
Cc: Yafang Shao <laoar.shao@...il.com>, 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>, 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 Kees,
On Wed, Aug 28, 2024 at 05:17:55PM GMT, Kees Cook wrote:
> On Wed, Aug 28, 2024 at 05:09:08PM +0200, Alejandro Colomar wrote:
> > Hi Kees,
> >
> > On Wed, Aug 28, 2024 at 06:48:39AM GMT, Kees Cook wrote:
> >
> > [...]
> >
> > > >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.
> > > >
> > > > 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))
> > >
> > > Uh, but why? strscpy() copies bytes, not array elements. Using sizeof() is already correct and using ARRAY_SIZE() could lead to unexpectedly small counts (in admittedly odd situations).
> > >
> > > What is the problem you're trying to solve here?
> >
> > I suggested that here:
> > <https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq/>
> >
> > There, you'll find the rationale (and also for avoiding the _pad calls
> > where not necessary --I ignore if it's necessary here--).
>
> Right, so we only use byte strings for strscpy(), so sizeof() is
> sufficient. There's no technical need to switch to ARRAY_SIZE(), and I'd
> like to minimize any changes to such core APIs without a good reason.
Makes sense. My original proposal was ignoring that the wrapper was
already using __must_be_array(). Having already sizeof() +
__must_be_array(), I'd leave it like that, since both do effectively the
same.
> And for the _pad change, we are also doing strncpy() replacement via
> case-by-case analysis, but with a common function like get_task_comm(),
> I don't want to change the behavior without a complete audit of the
> padding needs of every caller.
Agree. I had the same problem with shadow. Removing padding was the
worst part, because it was hard to justify that nothing was relying on
the padding.
> Since that's rather a lot for this series,
> I'd rather we just leave the existing behavior as-is, and if padding
> removal is wanted after that, we can do it on a case-by-case basis then.
>
> -Kees
Have a lovely night!
Alex
>
> --
> Kees Cook
--
<https://www.alejandro-colomar.es/>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists