Messages in this thread | | | Date | Tue, 18 Feb 2020 11:33:46 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH] vsprintf: don't obfuscate NULL and error pointers |
| |
On Tue 2020-02-18 01:07:53, Ilya Dryomov wrote: > On Tue, Feb 18, 2020 at 12:47 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Mon, Feb 17, 2020 at 11:28:03PM +0100, Ilya Dryomov wrote: > > > I don't see what security concern is addressed by obfuscating NULL > > > and IS_ERR() error pointers, printed with %p/%pK. Given the number > > > of sites where %p is used (over 10000) and the fact that NULL pointers > > > aren't uncommon, it probably wouldn't take long for an attacker to > > > find the hash that corresponds to 0. Although harder, the same goes > > > for most common error values, such as -1, -2, -11, -14, etc. > > > > > > The NULL part actually fixes a regression: NULL pointers weren't > > > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when > > > dereferencing invalid pointers") which went into 5.2. I'm tacking > > > the IS_ERR() part on here because error pointers won't leak kernel > > > addresses and printing them as pointers shouldn't be any different > > > from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes > > > debugging based on existing pr_debug and friends excruciating. > > > > > > Note that the "always print 0's for %pK when kptr_restrict == 2" > > > behaviour which goes way back is left as is. > > > > > > Example output with the patch applied: > > > > > > ptr error-ptr NULL > > > %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000 > > > %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000 > > > %px: ffff888048c04020 fffffffffffffff2 0000000000000000 > > > %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000 > > > %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000 > > > > This seems reasonable. Though I wonder -- since the efault string is > > exposed now -- should this instead print all the error-ptr strings > > instead of the unsigned negative pointer value?
It would make sense to distinguish it from a hashed value that might be in the NULL or ERR range as well.
The chance is small. But it might safe people from spending time on false paths.
That said, I am fine to accept the patch as is. It makes sense and it does not need to be perfect. After all, one motivation behind the hashed %p was to make it useless and motivate people to remove it. And I am sure that someone will send a patch adding error-ptr sooner or later anyway ;-)
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards, Petr
| |