linux-kernel - Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel

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]

Date:   Fri, 03 Apr 2020 21:05:26 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Christophe Leroy <christophe.leroy@....fr>,
        linuxppc-dev@...ts.ozlabs.org
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>, x86@...nel.org,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write
 without touching AMR

Christophe Leroy's on April 3, 2020 8:31 pm:
> 
> 
> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
> 
> I just discovered the following commit 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
> 
> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
> 
> When reading the commit log, I understand that probe_kernel_read() may 
> be used to access some user memory. Which will not work anymore with 
> your patch.

Hmm, I looked at _strict but obviously not hard enough. Good catch.

I don't think probe_kernel_read() should ever access user memory,
the comment certainly says it doesn't, but that patch sort of implies
that they do.

I think it's wrong. The non-_strict maybe could return userspace data to 
you if you did pass a user address? I don't see why that shouldn't just 
be disallowed always though.

And if the _strict version is required to be safe, then it seems like a
bug or security issue to just allow everyone that doesn't explicitly
override it to use the default implementation.

Also, the way the weak linkage is done in that patch, means parisc and
um archs that were previously overriding probe_kernel_read() now get
the default probe_kernel_read_strict(), which would be wrong for them.

> 
> Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that 
> you want to add ?
> 
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@...il.com>
>> ---
>> v2:
>> - Enable for all powerpc (suggested by Christophe)
>> - Fold helper function together (Christophe)
>> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>> 
>>   arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
>>   arch/powerpc/lib/Makefile          |  2 +-
>>   arch/powerpc/lib/maccess.c         | 34 ++++++++++++++++++++++++++++++
> 
> x86 does it in mm/maccess.c

Yeah I'll fix that up, thanks.

Thanks,
Nick