[PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[Sergey Shtylyov]

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 <s.shtylyov@omp.ru>

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

Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[Petr Mladek]

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 <s.shtylyov@omp.ru>
> 
> ---
> 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

Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[Steven Rostedt]

On Fri, 6 Jan 2023 16:49:46 +0100
Petr Mladek <pmladek@suse.com> 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;
> >    

Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[Linus Torvalds]

On Fri, Jan 6, 2023 at 7:49 AM Petr Mladek <pmladek@suse.com> 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

Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[Kees Cook]

On Fri, Jan 06, 2023 at 12:14:57PM -0500, Steven Rostedt wrote:
> On Fri, 6 Jan 2023 16:49:46 +0100
> Petr Mladek <pmladek@suse.com> 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

Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[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

Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

[Andy Shevchenko]

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