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@xxxxxxxxxx> > > > > Signed-off-by: Alejandro Colomar <alx@xxxxxxxxxx> > > > > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx> > > > > > > > >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/>
Attachment:
signature.asc
Description: PGP signature
