linux-kernel - Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190220102414.6rlwc77q7flxfjnp@pathway.suse.cz>
Date:   Wed, 20 Feb 2019 11:24:14 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Tobin C . Harding" <me@...in.cc>, Joe Perches <joe@...ches.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.cz>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing
 invalid pointers

On Tue 2019-02-19 23:15:16, Sergey Senozhatsky wrote:
> On (02/19/19 15:49), Andy Shevchenko wrote:
> > > On (02/19/19 13:02), Andy Shevchenko wrote:
> > > [..]
> > > > And if it's not? You will get in either case incomplete information,
> > > > but at least with "(e" (or even "(") you might get a clue that it
> > > > errornous conditions.
> > > 
> > > The thing I'm signaling here is that in some cases we still can
> > > crash the kernel; with the difference that invalid dereference
> > > can now be a memory corruption. Just saying.
> > 
> > Wouldn't that mean that the culprit in the caller, not in the callee?
> > 
> > (As far as I got your another example with badly called sprintf() which may
> >  overwrite stack, etc).
> 
> ipv4 printout case does not look like a caller bug to me: we expect a 15
> bytes ipv4 address, allocate a 16 bytes buffer, sprintf() voluntarily
> writes 18 bytes. This error reporting is a bit of a dangerous practice;
> next year someone might add another specifier and another
> 
> 	return string(buf, end, "(this data does not look right)", spec);

I hope that this would never pass a review.

> We probably would want to do something about it. For instance, mandating
> that "(error)" string cannot be larger than 8 bytes can be a good starting
> point.

All the warnings are printed by string_nocheck(). We could add one
more variant for printing the warnings, for example:

static int error_string(char **buf, char *end, const char *err_msg,
			     struct printf_spec spec)
{
	/*
	 * Limit the error message when the buffer size is unknown.
	 * It is not full proof but it looks like a reasonable
	 * compromise. The (null) error string never caused a problem.
	 */
	if ((end - *buf > INT_MAX / 2) && !spec.precision) {
		if (spec.field_width)
			spec.precision = spec.field_width;
		else
			spec.precision = 8;
	}

	*buf = string_nocheck(*buf, end, err_msg, spec);
		return -EFAULT;
	}

	return 0;
}

Well, I am not sure that it is worth it. I guess that "(null)"
never caused a problem. And all the new error messages have
8 characters at maximum.

Best Regards,
Petr