linux-kernel - RE: [PATCH] microblaze: remove CONFIG_SET

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]

Date:   Thu, 10 Feb 2022 14:21:07 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Arnd Bergmann' <arnd@...nel.org>
CC:     Michal Simek <monstr@...str.eu>,
        Christoph Hellwig <hch@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] microblaze: remove CONFIG_SET_FS

From: Arnd Bergmann
> Sent: 10 February 2022 13:30
...
> #define __access_ok(addr, size) \
>         ((get_fs().seg == KERNEL_DS.seg) || \
>         (((unsigned long)addr < get_fs().seg) && \
>           (unsigned long)size < (get_fs().seg - (unsigned long)addr)))

That one is strange.
Seems to be optimised for kernel accesses!

> ia64 and sparc skip the size check entirely but rely on an unmapped page
> at the beginning of the kernel address range, which avoids this problem
> but may also be dangerous.

An unmapped page requires that the kernel do sequential accesses
(or, at least, not big offset) - which is normally fine.
Especially for 64bit where there is plenty of address space.
I guess it could be problematic for 32bit if you can/want to
use 'big pages' for the kernel addresses.
Losing a single (typically) 4k page isn't a problem.

Certainly not mapping the page at TASK_SIZE is a good safety check.
Actually, setting TASK_SIZE to 0xc0000000 - PAGE_SIZE and never
mapping the last user page has the same effect.
Except I bet the ldso has to go there :-(
Not to mention the instruction sets where loading the constant
would then be two instructions.

...
> > Although typical 64bit architectures can use the faster:
> >         ((addr | size) >> 62)
> 
> I think this is the best version, and it's already widely used:

I just did a quick check, both clang and gcc optimise out
constant values for 'size'.

> static inline int __range_ok(unsigned long addr, unsigned long size)
> {
>         return size <= TASK_SIZE && addr <= (TASK_SIZE - size);
> }
> 
> since 'size' is usually constant, so this turns into a single comparison
> against a compile-time constant.

Hmmm... maybe there should be a comment that it is the same as
the more obvious:
	(addr <= TASK_SIZE && addr <= TASK_SIZE - size)
but is better for constant size.
(Provided TASK_SIZE is a constant.)

I'm sure Linus was 'unhappy' about checking against 2^63 for
32bit processes on a 64bit kernel.

Hmmm compat code that has 32bit addr/size needn't even call
access_ok() - it can never access kernel memory at all.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)