In vsnprintf() etc, C99 allows the 'buf' argument to be NULL when the
'size' argument equals 0. Let us treat NULL passed as if the 'buf'
argument pointed to a 0-sized buffer, so that we can avoid a NULL pointer
dereference and still return the # of characters that would be written if
'buf' pointed to a valid buffer...
Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.
Signed-off-by: Sergey Shtylyov <[email protected]>
---
This patch is against the 'master' branch of the PRINTK Group's repo...
lib/vsprintf.c | 9 +++++++++
1 file changed, 9 insertions(+)
Index: linux/lib/vsprintf.c
===================================================================
--- linux.orig/lib/vsprintf.c
+++ linux/lib/vsprintf.c
@@ -2738,6 +2738,15 @@ int vsnprintf(char *buf, size_t size, co
if (WARN_ON_ONCE(size > INT_MAX))
return 0;
+ /*
+ * C99 allows @buf to be NULL when @size is 0. We treat such NULL as if
+ * @buf pointed to 0-sized buffer, so we can both avoid a NULL pointer
+ * dereference and still return # of characters that would be written
+ * if @buf pointed to a valid buffer...
+ */
+ if (!buf)
+ size = 0;
+
str = buf;
end = buf + size;
Hi,
Adding Kees, Linus, and linux-hardening into Cc. It seems that
__vsnprintf_internal() does not do this in glibc. I wonder if
there is a good reason for it.
My guess is that glibc does not do this either because they do not
want to quietly hide bugs or just nobody had this idea.
On Fri 2023-01-06 00:16:31, Sergey Shtylyov wrote:
> In vsnprintf() etc, C99 allows the 'buf' argument to be NULL when the
> 'size' argument equals 0. Let us treat NULL passed as if the 'buf'
> argument pointed to a 0-sized buffer, so that we can avoid a NULL pointer
> dereference and still return the # of characters that would be written if
> 'buf' pointed to a valid buffer...
This is misleading. The message says how vsprintf() should behave
by the standard. But it does not describe what this patch does.
The function already behaves by the standard. The change is that
the vsprintf() will catch an obvious mistake and prevent failure.
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
>
> Signed-off-by: Sergey Shtylyov <[email protected]>
>
> ---
> This patch is against the 'master' branch of the PRINTK Group's repo...
>
> lib/vsprintf.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> Index: linux/lib/vsprintf.c
> ===================================================================
> --- linux.orig/lib/vsprintf.c
> +++ linux/lib/vsprintf.c
> @@ -2738,6 +2738,15 @@ int vsnprintf(char *buf, size_t size, co
> if (WARN_ON_ONCE(size > INT_MAX))
> return 0;
>
> + /*
> + * C99 allows @buf to be NULL when @size is 0. We treat such NULL as if
> + * @buf pointed to 0-sized buffer, so we can both avoid a NULL pointer
> + * dereference and still return # of characters that would be written
> + * if @buf pointed to a valid buffer...
> + */
> + if (!buf)
> + size = 0;
It makes sense except that it would hide bugs. It should print a
warning, for example:
char *err_msg;
err_msg = check_pointer_msg(buf);
if (err_msg) {
WARN_ONCE(1, "Invalid buffer passed to vsnprintf(). Trying to continue with 0 length limit\n");
size = 0;
}
check_pointer_msg() allows to catch even more buggy pointers. WARN()
helps to locate the caller. WARN_ONCE() variant is used to prevent
a potential infinite loop.
> +
> str = buf;
> end = buf + size;
>
Best Regards,
Petr
On Fri, 6 Jan 2023 16:49:46 +0100
Petr Mladek <[email protected]> wrote:
> > Index: linux/lib/vsprintf.c
> > ===================================================================
> > --- linux.orig/lib/vsprintf.c
> > +++ linux/lib/vsprintf.c
> > @@ -2738,6 +2738,15 @@ int vsnprintf(char *buf, size_t size, co
> > if (WARN_ON_ONCE(size > INT_MAX))
> > return 0;
> >
> > + /*
> > + * C99 allows @buf to be NULL when @size is 0. We treat such NULL as if
> > + * @buf pointed to 0-sized buffer, so we can both avoid a NULL pointer
> > + * dereference and still return # of characters that would be written
> > + * if @buf pointed to a valid buffer...
> > + */
> > + if (!buf)
> > + size = 0;
>
> It makes sense except that it would hide bugs. It should print a
> warning, for example:
I agree. This is a bug, and should not be quietly ignored.
>
> char *err_msg;
>
> err_msg = check_pointer_msg(buf);
> if (err_msg) {
> WARN_ONCE(1, "Invalid buffer passed to vsnprintf(). Trying to continue with 0 length limit\n");
> size = 0;
> }
if (WARN_ONCE(err_msg, "Invalid buffer passed to vsnprintf(). Trying to continue with 0 length limit\n"))
size = 0;
;-)
-- Steve
>
> check_pointer_msg() allows to catch even more buggy pointers. WARN()
> helps to locate the caller. WARN_ONCE() variant is used to prevent
> a potential infinite loop.
>
> > +
> > str = buf;
> > end = buf + size;
> >
On Fri, Jan 06, 2023 at 12:14:57PM -0500, Steven Rostedt wrote:
> On Fri, 6 Jan 2023 16:49:46 +0100
> Petr Mladek <[email protected]> wrote:
>
> > > Index: linux/lib/vsprintf.c
> > > ===================================================================
> > > --- linux.orig/lib/vsprintf.c
> > > +++ linux/lib/vsprintf.c
> > > @@ -2738,6 +2738,15 @@ int vsnprintf(char *buf, size_t size, co
> > > if (WARN_ON_ONCE(size > INT_MAX))
> > > return 0;
> > >
> > > + /*
> > > + * C99 allows @buf to be NULL when @size is 0. We treat such NULL as if
> > > + * @buf pointed to 0-sized buffer, so we can both avoid a NULL pointer
> > > + * dereference and still return # of characters that would be written
> > > + * if @buf pointed to a valid buffer...
> > > + */
> > > + if (!buf)
> > > + size = 0;
> >
> > It makes sense except that it would hide bugs. It should print a
> > warning, for example:
>
> I agree. This is a bug, and should not be quietly ignored.
Yup.
>
> >
> > char *err_msg;
> >
> > err_msg = check_pointer_msg(buf);
> > if (err_msg) {
> > WARN_ONCE(1, "Invalid buffer passed to vsnprintf(). Trying to continue with 0 length limit\n");
> > size = 0;
> > }
>
> if (WARN_ONCE(err_msg, "Invalid buffer passed to vsnprintf(). Trying to continue with 0 length limit\n"))
> size = 0;
Also good. Please CC me for an Ack when this is a full patch. :)
-Kees
--
Kees Cook
On Fri, Jan 6, 2023 at 7:49 AM Petr Mladek <[email protected]> wrote:
>
> Adding Kees, Linus, and linux-hardening into Cc. It seems that
> __vsnprintf_internal() does not do this in glibc. I wonder if
> there is a good reason for it.
I do not think that patch is valid.
And I don't like Steven Rostedt's suggestion either.
I think that code *should* take a NULL pointer dereference, exactly
the same way "strlen()" would do if you pass it a NULL pointer and
claim there is room there.
No silly checks for invalid cases.
There's any number of invalid things you can do in the kernel, and we
don't check for those either.
I don't particularly like the "pass NULL to sprintf()" thing at all,
but *if* somebody does, they had better just pass a zero size too.
And doing
git grep 'sn*printf(NULL'
seems to show that all current users do exactly that.
If somebody wants to check for this, make it a coccinelle script or
something. Not a runtime check for invalid use cases.
Linus
On Mon, Jan 09, 2023 at 01:23:40PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 06, 2023 at 12:16:31AM +0300, Sergey Shtylyov wrote:
...
> > + /*
> > + * C99 allows @buf to be NULL when @size is 0. We treat such NULL as if
> > + * @buf pointed to 0-sized buffer, so we can both avoid a NULL pointer
> > + * dereference and still return # of characters that would be written
> > + * if @buf pointed to a valid buffer...
> > + */
> > + if (!buf)
> > + size = 0;
>
> Do we have test cases for that?
This still stays...
> And what's wrong to print "(null)" ? Have you checked if your patch makes any
> regressions to those cases?
...but this paragraph is not for the case (I mixed it with the arguments to be
NULL).
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 06, 2023 at 12:16:31AM +0300, Sergey Shtylyov wrote:
> In vsnprintf() etc, C99 allows the 'buf' argument to be NULL when the
> 'size' argument equals 0. Let us treat NULL passed as if the 'buf'
> argument pointed to a 0-sized buffer, so that we can avoid a NULL pointer
> dereference and still return the # of characters that would be written if
> 'buf' pointed to a valid buffer...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
...
> + /*
> + * C99 allows @buf to be NULL when @size is 0. We treat such NULL as if
> + * @buf pointed to 0-sized buffer, so we can both avoid a NULL pointer
> + * dereference and still return # of characters that would be written
> + * if @buf pointed to a valid buffer...
> + */
> + if (!buf)
> + size = 0;
Do we have test cases for that?
And what's wrong to print "(null)" ? Have you checked if your patch makes any
regressions to those cases?
--
With Best Regards,
Andy Shevchenko