Re: [PATCH v5 0/9] Improve the copy of task comm

[Alejandro Colomar]

Hi Yafang,

On Thu, Aug 08, 2024 at 10:49:17AM GMT, Yafang Shao wrote:
> > > Now, it might be a good idea to also verify that 'buf' is an actual
> > > array, and that this code doesn't do some silly "sizeof(ptr)" thing.
> >
> > I decided to use NITEMS() instead of sizeof() for that reason.
> > (NITEMS() is just our name for ARRAY_SIZE().)
> >
> > $ grepc -h NITEMS .
> > #define NITEMS(a)(SIZEOF_ARRAY((a)) / sizeof((a)[0]))
> >
> > > We do have a helper for that, so we could do something like
> > >
> > >#define get_task_comm(buf, tsk) \
> > > strscpy_pad(buf, __must_be_array(buf)+sizeof(buf), (tsk)->comm)
> >
> > We have SIZEOF_ARRAY() for when you want the size of an array:
> >
> > $ grepc -h SIZEOF_ARRAY .
> > #define SIZEOF_ARRAY(a)  (sizeof(a) + must_be_array(a))
> 
> There is already a similar macro in Linux:
> 
>   /**
>* ARRAY_SIZE - get the number of elements in array @arr
>* @arr: array to be sized
>*/
>   #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))

This is actually the same as our NITEMS(), not SIZEOF_ARRAY().

> will use it instead of the sizeof().

But yeah, indeed I think you should use ARRAY_SIZE() in
get_task_comm().  :)

> 
> Good point.
> I will avoid using the _pad().

Nice.  :)

Have a lovely day!
Alex

-- 



signature.asc
Description: PGP signature

Re: [PATCH v5 0/9] Improve the copy of task comm

[Yafang Shao]

On Wed, Aug 7, 2024 at 1:28 AM Alejandro Colomar  wrote:
>
> Hi Linus,
>
> Serge let me know about this thread earlier today.
>
> On 2024-08-05, Linus Torvalds  wrote:
> > On Mon, 5 Aug 2024 at 20:01, Yafang Shao  wrote:
> > >
> > > One concern about removing the BUILD_BUG_ON() is that if we extend
> > > TASK_COMM_LEN to a larger size, such as 24, the caller with a
> > > hardcoded 16-byte buffer may overflow.
> >
> > No, not at all. Because get_task_comm() - and the replacements - would
> > never use TASK_COMM_LEN.
> >
> > They'd use the size of the *destination*. That's what the code already does:
> >
> >   #define get_task_comm(buf, tsk) ({  \
> >   ...
> > __get_task_comm(buf, sizeof(buf), tsk); \
> >
> > note how it uses "sizeof(buf)".
>
> In shadow.git, we also implemented macros that are named after functions
> and calculate the appropriate number of elements internally.
>
> $ grepc -h STRNCAT .
> #define STRNCAT(dst, src)  strncat(dst, src, NITEMS(src))
> $ grepc -h STRNCPY .
> #define STRNCPY(dst, src)  strncpy(dst, src, NITEMS(dst))
> $ grepc -h STRTCPY .
> #define STRTCPY(dst, src)  strtcpy(dst, src, NITEMS(dst))
> $ grepc -h STRFTIME .
> #define STRFTIME(dst, fmt, tm)  strftime(dst, NITEMS(dst), fmt, tm)
> $ grepc -h DAY_TO_STR .
> #define DAY_TO_STR(str, day, iso)   day_to_str(NITEMS(str), str, day, 
> iso)
>
> They're quite useful, and when implementing them we found and fixed
> several bugs thanks to them.
>
> > Now, it might be a good idea to also verify that 'buf' is an actual
> > array, and that this code doesn't do some silly "sizeof(ptr)" thing.
>
> I decided to use NITEMS() instead of sizeof() for that reason.
> (NITEMS() is just our name for ARRAY_SIZE().)
>
> $ grepc -h NITEMS .
> #define NITEMS(a)(SIZEOF_ARRAY((a)) / sizeof((a)[0]))
>
> > We do have a helper for that, so we could do something like
> >
> >#define get_task_comm(buf, tsk) \
> > strscpy_pad(buf, __must_be_array(buf)+sizeof(buf), (tsk)->comm)
>
> We have SIZEOF_ARRAY() for when you want the size of an array:
>
> $ grepc -h SIZEOF_ARRAY .
> #define SIZEOF_ARRAY(a)  (sizeof(a) + must_be_array(a))

There is already a similar macro in Linux:

  /**
   * ARRAY_SIZE - get the number of elements in array @arr
   * @arr: array to be sized
   */
  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))

will use it instead of the sizeof().

>
> However, I don't think you want sizeof().  Let me explain why:
>
> -  Let's say you want to call wcsncpy(3) (I know nobody should be using
>that function, not strncpy(3), but I'm using it as a standard example
>of a wide-character string function).
>
>You should call wcsncpy(dst, src, NITEMS(dst)).
>A call wcsncpy(dst, src, sizeof(dst)) is bogus, since the argument is
>the number of wide characters, not the number of bytes.
>
>When translating that to normal characters, you want conceptually the
>same operation, but on (normal) characters.  That is, you want
>strncpy(dst, src, NITEMS(dst)).  While strncpy(3) with sizeof() works
>just fine because sizeof(char)==1 by definition, it is conceptually
>wrong to use it.
>
>By using NITEMS() (i.e., ARRAY_SIZE()), you get the __must_be_array()
>check for free.
>
> In the end, SIZEOF_ARRAY() is something we very rarely use.  It's there
> only used in the following two cases at the moment:
>
> #define NITEMS(a)(SIZEOF_ARRAY((a)) / sizeof((a)[0]))
> #define MEMZERO(arr)  memzero(arr, SIZEOF_ARRAY(arr))
>
> Does that sound convincing?
>
> For memcpy(3) for example, you do want sizeof(), because you're copying
> raw bytes, but with strings, in which characters are conceptually
> meaningful elements, NITEMS() makes more sense.
>
> BTW, I'm working on a __lengthof__ operator that will soon allow using
> it on function parameters declared with array notation.  That is,
>
> size_t
> f(size_t n, int a[n])
> {
> return __lengthof__(a);  // This will return n.
> }
>
> If you're interested in it, I'm developing and discussing it here:
> 
>
> >
> > as a helper macro for this all.
> >
> > (Although I'm not convinced we generally want the "_pad()" version,
> > but whatever).
>
> We had problems with it in shadow recently.  In user-space, it's similar
> to strncpy(3) (at least if you wrap it in a macro that makes sure that
> it terminates the string with a null byte).
>
> We had a lot of uses of strncpy(3), from old times where that was used
> to copy strings with truncation.  I audited all of that code (and
> haven't really finished yet), and translated to calls similar to
> strscpy(9) (we call it strtcpy(), as it _t_runcates).  The problem was
> that in some cases 

Re: [PATCH v5 0/9] Improve the copy of task comm

[Alejandro Colomar]

Hi Linus,

Serge let me know about this thread earlier today.

On 2024-08-05, Linus Torvalds  wrote:
> On Mon, 5 Aug 2024 at 20:01, Yafang Shao  wrote:
> >
> > One concern about removing the BUILD_BUG_ON() is that if we extend
> > TASK_COMM_LEN to a larger size, such as 24, the caller with a
> > hardcoded 16-byte buffer may overflow.
> 
> No, not at all. Because get_task_comm() - and the replacements - would
> never use TASK_COMM_LEN.
> 
> They'd use the size of the *destination*. That's what the code already does:
> 
>   #define get_task_comm(buf, tsk) ({  \
>   ...
> __get_task_comm(buf, sizeof(buf), tsk); \
> 
> note how it uses "sizeof(buf)".

In shadow.git, we also implemented macros that are named after functions
and calculate the appropriate number of elements internally.

$ grepc -h STRNCAT .
#define STRNCAT(dst, src)  strncat(dst, src, NITEMS(src))
$ grepc -h STRNCPY .
#define STRNCPY(dst, src)  strncpy(dst, src, NITEMS(dst))
$ grepc -h STRTCPY .
#define STRTCPY(dst, src)  strtcpy(dst, src, NITEMS(dst))
$ grepc -h STRFTIME .
#define STRFTIME(dst, fmt, tm)  strftime(dst, NITEMS(dst), fmt, tm)
$ grepc -h DAY_TO_STR .
#define DAY_TO_STR(str, day, iso)   day_to_str(NITEMS(str), str, day, 
iso)

They're quite useful, and when implementing them we found and fixed
several bugs thanks to them.

> Now, it might be a good idea to also verify that 'buf' is an actual
> array, and that this code doesn't do some silly "sizeof(ptr)" thing.

I decided to use NITEMS() instead of sizeof() for that reason.
(NITEMS() is just our name for ARRAY_SIZE().)

$ grepc -h NITEMS .
#define NITEMS(a)(SIZEOF_ARRAY((a)) / sizeof((a)[0]))

> We do have a helper for that, so we could do something like
> 
>#define get_task_comm(buf, tsk) \
> strscpy_pad(buf, __must_be_array(buf)+sizeof(buf), (tsk)->comm)

We have SIZEOF_ARRAY() for when you want the size of an array:

$ grepc -h SIZEOF_ARRAY .
#define SIZEOF_ARRAY(a)  (sizeof(a) + must_be_array(a))

However, I don't think you want sizeof().  Let me explain why:

-  Let's say you want to call wcsncpy(3) (I know nobody should be using
   that function, not strncpy(3), but I'm using it as a standard example
   of a wide-character string function).

   You should call wcsncpy(dst, src, NITEMS(dst)).
   A call wcsncpy(dst, src, sizeof(dst)) is bogus, since the argument is
   the number of wide characters, not the number of bytes.

   When translating that to normal characters, you want conceptually the
   same operation, but on (normal) characters.  That is, you want
   strncpy(dst, src, NITEMS(dst)).  While strncpy(3) with sizeof() works
   just fine because sizeof(char)==1 by definition, it is conceptually
   wrong to use it.

   By using NITEMS() (i.e., ARRAY_SIZE()), you get the __must_be_array()
   check for free.

In the end, SIZEOF_ARRAY() is something we very rarely use.  It's there
only used in the following two cases at the moment:

#define NITEMS(a)(SIZEOF_ARRAY((a)) / sizeof((a)[0]))
#define MEMZERO(arr)  memzero(arr, SIZEOF_ARRAY(arr))

Does that sound convincing?

For memcpy(3) for example, you do want sizeof(), because you're copying
raw bytes, but with strings, in which characters are conceptually
meaningful elements, NITEMS() makes more sense.

BTW, I'm working on a __lengthof__ operator that will soon allow using
it on function parameters declared with array notation.  That is,

size_t
f(size_t n, int a[n])
{
return __lengthof__(a);  // This will return n.
}

If you're interested in it, I'm developing and discussing it here:


> 
> as a helper macro for this all.
> 
> (Although I'm not convinced we generally want the "_pad()" version,
> but whatever).

We had problems with it in shadow recently.  In user-space, it's similar
to strncpy(3) (at least if you wrap it in a macro that makes sure that
it terminates the string with a null byte).

We had a lot of uses of strncpy(3), from old times where that was used
to copy strings with truncation.  I audited all of that code (and
haven't really finished yet), and translated to calls similar to
strscpy(9) (we call it strtcpy(), as it _t_runcates).  The problem was
that in some cases the padding was necessary, and in others it was not,
and it was very hard to distinguish those.

I recommend not zeroing strings unnecessarily, since that will make it
hard to review the code later.  E.g., twenty years from now, someone
takes a piece of code with a _pad() call, and has no clue if the zeroing
was for a reason, or for no reason.

On the other hand, not zeroing may make it easier to explot bugs, so
whatever you think best.  In the kernel you may need to be more worried
than in user space.  Whatever.  :)

> 

Re: [PATCH v5 0/9] Improve the copy of task comm

[Yafang Shao]

On Tue, Aug 6, 2024 at 11:10 AM Linus Torvalds
 wrote:
>
> On Mon, 5 Aug 2024 at 20:01, Yafang Shao  wrote:
> >
> > One concern about removing the BUILD_BUG_ON() is that if we extend
> > TASK_COMM_LEN to a larger size, such as 24, the caller with a
> > hardcoded 16-byte buffer may overflow.
>
> No, not at all. Because get_task_comm() - and the replacements - would
> never use TASK_COMM_LEN.
>
> They'd use the size of the *destination*. That's what the code already does:
>
>   #define get_task_comm(buf, tsk) ({  \
>   ...
> __get_task_comm(buf, sizeof(buf), tsk); \
>
> note how it uses "sizeof(buf)".
>
> Now, it might be a good idea to also verify that 'buf' is an actual
> array, and that this code doesn't do some silly "sizeof(ptr)" thing.
>
> We do have a helper for that, so we could do something like
>
>#define get_task_comm(buf, tsk) \
> strscpy_pad(buf, __must_be_array(buf)+sizeof(buf), (tsk)->comm)
>
> as a helper macro for this all.
>
> (Although I'm not convinced we generally want the "_pad()" version,
> but whatever).
>

Will do it.
Thanks for your explanation.

-- 
Regards
Yafang

Re: [PATCH v5 0/9] Improve the copy of task comm

[Linus Torvalds]

On Mon, 5 Aug 2024 at 20:01, Yafang Shao  wrote:
>
> One concern about removing the BUILD_BUG_ON() is that if we extend
> TASK_COMM_LEN to a larger size, such as 24, the caller with a
> hardcoded 16-byte buffer may overflow.

No, not at all. Because get_task_comm() - and the replacements - would
never use TASK_COMM_LEN.

They'd use the size of the *destination*. That's what the code already does:

  #define get_task_comm(buf, tsk) ({  \
  ...
__get_task_comm(buf, sizeof(buf), tsk); \

note how it uses "sizeof(buf)".

Now, it might be a good idea to also verify that 'buf' is an actual
array, and that this code doesn't do some silly "sizeof(ptr)" thing.

We do have a helper for that, so we could do something like

   #define get_task_comm(buf, tsk) \
strscpy_pad(buf, __must_be_array(buf)+sizeof(buf), (tsk)->comm)

as a helper macro for this all.

(Although I'm not convinced we generally want the "_pad()" version,
but whatever).

Linus

Re: [PATCH v5 0/9] Improve the copy of task comm

[Yafang Shao]

On Tue, Aug 6, 2024 at 5:28 AM Linus Torvalds
 wrote:
>
> On Sun, 4 Aug 2024 at 00:56, Yafang Shao  wrote:
> >
> > There is a BUILD_BUG_ON() inside get_task_comm(), so when you use
> > get_task_comm(), it implies that the BUILD_BUG_ON() is necessary.
>
> Let's just remove that silly BUILD_BUG_ON(). I don't think it adds any
> value, and honestly, it really only makes this patch-series uglier
> when reasonable uses suddenly pointlessly need that double-underscore
> version..
>
> So let's aim at
>
>  (a) documenting that the last byte in 'tsk->comm{}' is always
> guaranteed to be NUL, so that the thing can always just be treated as
> a string. Yes, it may change under us, but as long as we know there is
> always a stable NUL there *somewhere*, we really really don't care.
>
>  (b) removing __get_task_comm() entirely, and replacing it with a
> plain 'str*cpy*()' functions
>
> The whole (a) thing is a requirement anyway, since the *bulk* of
> tsk->comm really just seems to be various '%s' things in printk
> strings etc.
>
> And once we just admit that we can use the string functions, all the
> get_task_comm() stuff is just unnecessary.
>
> And yes, some people may want to use the strscpy_pad() function
> because they want to fill the whole destination buffer. But that's
> entirely about the *destination* use, not the tsk->comm[] source, so
> it has nothing to do with any kind of "get_task_comm()" logic, and it
> was always wrong to care about the buffer sizes magically matching.
>
> Hmm?

One concern about removing the BUILD_BUG_ON() is that if we extend
TASK_COMM_LEN to a larger size, such as 24, the caller with a
hardcoded 16-byte buffer may overflow. This could be an issue with
code in include/linux/elfcore.h and include/linux/elfcore-compat.h,
posing a risk. However, I believe it is the caller's responsibility to
explicitly add a null terminator if it uses a fixed buffer that cannot
be changed. Therefore, the following code change is necessary:

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5ae8045f4df4..e4b0b7cf0c1f 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1579,6 +1579,7 @@ static int fill_psinfo(struct elf_prpsinfo
*psinfo, struct task_struct *p,
SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid));
rcu_read_unlock();
get_task_comm(psinfo->pr_fname, p);
+   psinfo->pr_fname[15] = '\0';

return 0;
 }

However, it is currently safe to remove the BUILD_BUG_ON() since
TASK_COMM_LEN is still 16.

--
Regards
Yafang

Re: [PATCH v5 0/9] Improve the copy of task comm

[Linus Torvalds]

On Sun, 4 Aug 2024 at 00:56, Yafang Shao  wrote:
>
> There is a BUILD_BUG_ON() inside get_task_comm(), so when you use
> get_task_comm(), it implies that the BUILD_BUG_ON() is necessary.

Let's just remove that silly BUILD_BUG_ON(). I don't think it adds any
value, and honestly, it really only makes this patch-series uglier
when reasonable uses suddenly pointlessly need that double-underscore
version..

So let's aim at

 (a) documenting that the last byte in 'tsk->comm{}' is always
guaranteed to be NUL, so that the thing can always just be treated as
a string. Yes, it may change under us, but as long as we know there is
always a stable NUL there *somewhere*, we really really don't care.

 (b) removing __get_task_comm() entirely, and replacing it with a
plain 'str*cpy*()' functions

The whole (a) thing is a requirement anyway, since the *bulk* of
tsk->comm really just seems to be various '%s' things in printk
strings etc.

And once we just admit that we can use the string functions, all the
get_task_comm() stuff is just unnecessary.

And yes, some people may want to use the strscpy_pad() function
because they want to fill the whole destination buffer. But that's
entirely about the *destination* use, not the tsk->comm[] source, so
it has nothing to do with any kind of "get_task_comm()" logic, and it
was always wrong to care about the buffer sizes magically matching.

Hmm?

Linus