Re: [PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values

From: Romain Perier <romain.perier@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>,
	 Kernel Hardening <kernel-hardening@lists.openwall.com>,
	Ingo Molnar <mingo@redhat.com>,
	 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values
Date: Thu, 25 Feb 2021 09:38:48 +0100	[thread overview]
Message-ID: <CABgxDo+P4WqdzUz_OY1cVvcScnzZA3b7+Opz53aTf=A-9ZZBtw@mail.gmail.com> (raw)
In-Reply-To: <20210222124936.03103585@gandalf.local.home>

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

Le lun. 22 févr. 2021 à 18:49, Steven Rostedt <rostedt@goodmis.org> a
écrit :

> > -     if (unlikely(!maxlen))
> > -             return -ENOMEM;
>
> Don't remove the above. You just broke the else side.
>
> > -
> > -     if (addr == FETCH_TOKEN_COMM)
> > -             ret = strlcpy(dst, current->comm, maxlen);
> > -     else
> > +     if (addr == FETCH_TOKEN_COMM) {
> > +             ret = strscpy(dst, current->comm, maxlen);
> > +             if (ret == -E2BIG)
> > +                     return -ENOMEM;
>
> I'm not sure the above is what we want. current->comm is always nul
> terminated, and not only that, it will never be bigger than TASK_COMM_LEN.
> If the "dst" location is smaller than comm (maxlen < TASK_COMM_LEN), it is
> still OK to copy a partial string. It should not return -ENOMEM which looks
> to be what happens with this patch.
>
> In other words, it looks like this patch breaks the current code in more
> ways than one.
>
> -- Steve
>

Hello,

Mhhh, *I think* that I had an issue during rebase, I don't remember to have
removed the "  if (unlikely(!maxlen))"  (sorry for that).
Well, strscpy always returns a truncated string even in case of possible
overflow, the function copies what it can in "dst", it will just return
-E2BIG when
it does not fit or when "count" has a bad value (zero or > INT_MAX). We
have just to make a difference between "-E2BIG, data has been copied to dst
and it is truncated" and "-E2BIG, possible wrong size passed as argument".

I agree that it needs at least to work like before, and I think we can
preserve the old behaviour even with strscpy (we just need to adapt the
error handling accordingly).
I will fix this in v2.

Thanks,
Romain

[-- Attachment #2: Type: text/html, Size: 2415 bytes --]