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:

quoted

Hi Kees,

On Wed, Aug 28, 2024 at 06:48:39AM GMT, Kees Cook wrote:

[...]

quoted

quoted

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 [off-list ref]
   Signed-off-by: Alejandro Colomar [off-list ref]
   Signed-off-by: Yafang Shao [off-list ref]

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/ (local)>

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/>