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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y7hDGoCbWG2ZYdlV@alley>
Date:   Fri, 6 Jan 2023 16:49:46 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Shtylyov <s.shtylyov@....ru>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Kees Cook <keescook@...omium.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vsprintf: fix possible NULL pointer deref in vsnprintf()

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@....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