linux-kernel - Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr

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: <20180405143412.xuict2cdttr7dzh4@pathway.suse.cz>
Date:   Thu, 5 Apr 2018 16:34:12 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        "Tobin C . Harding" <me@...in.cc>, Joe Perches <joe@...ches.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.cz>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for
 kptr_restrict == 0

On Thu 2018-04-05 08:10:14, Sergey Senozhatsky wrote:
> On (04/04/18 10:58), Petr Mladek wrote:
> >
> > restricted_pointer() pretends that it prints the address when kptr_restrict
> > is set to zero. But it is never called in this situation. Instead,
> > pointer() falls back to ptr_to_id() and hashes the pointer.
> > 
> > This patch removes the potential confusion. klp_restrict is checked only
> > in restricted_pointer().
> > 
> > It should actually fix a small race when the address might get printed
> > unhashed:
> 
> Early morning, didn't have my coffee yet [like really didn't].
> 
> But I don't see how you "fix" a race. "echo 0" might still be called
> later than switch().

I assume that switch() is safe enough and we always have to take one
path there. The old code allowed to print the original pointer without
the capability check (case 0). The new code does:

    case 0: print hashed pointer
    case 1: keep original pointer only with sufficient privileges
    case 2:
    default: set NULL

The old code expected that it would never use case 0. But it was
possible because of the race.


> [..]
> > @@ -1426,8 +1427,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
> >  
> >  	switch (kptr_restrict) {
> >  	case 0:
> > -		/* Always print %pK values */
> > -		break;
> > +		/* Handle as %p, hash and do _not_ leak addresses. */
> > +		return ptr_to_id(buf, end, ptr, spec);
> 
> >From "Always print pK values" to "Always print hashed values"... Do we need
> %pK then? You probably need to update printk-formats.rst as well.

It is already correctly documented in Documentation/sysctl/kernel.txt.
printk-formats.rst uses reference to it.

Best Regards,
Petr