Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Laight <David.Laight@ACULAB.COM>
To: 'Christophe Leroy' <christophe.leroy@csgroup.eu>,
	'Christoph Hellwig' <hch@lst.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Michael Ellerman <mpe@ellerman.id.au>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	Kees Cook <keescook@chromium.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
Date: Wed, 2 Sep 2020 15:02:00 +0000	[thread overview]
Message-ID: <1599b80426ec4759b5c1beb9d9543fdc@AcuMS.aculab.com> (raw)
In-Reply-To: <6e88048a-8b30-400e-11c6-8d91ba77cbb0@csgroup.eu>

From: Christophe Leroy
> Sent: 02 September 2020 15:13
> 
> 
> Le 02/09/2020 à 15:51, David Laight a écrit :
> > From: Christophe Leroy
> >> Sent: 02 September 2020 14:25
> >> Le 02/09/2020 à 15:13, David Laight a écrit :
> >>> From: Christoph Hellwig
> >>>> Sent: 02 September 2020 13:37
> >>>>
> >>>> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
> >>>>>> -		return 0;
> >>>>>> -	return (size == 0 || size - 1 <= seg.seg - addr);
> >>>>>> +	if (addr >= TASK_SIZE_MAX)
> >>>>>> +		return false;
> >>>>>> +	if (size == 0)
> >>>>>> +		return false;
> >>>>>
> >>>>> __access_ok() was returning true when size == 0 up to now. Any reason to
> >>>>> return false now ?
> >>>>
> >>>> No, this is accidental and broken.  Can you re-run your benchmark with
> >>>> this fixed?
> >>>
> >>> Is TASK_SIZE_MASK defined such that you can do:
> >>>
> >>> 	return (addr | size) < TASK_SIZE_MAX) || !size;
> >>
> >> TASK_SIZE_MAX will usually be 0xc0000000
> >>
> >> With:
> >> addr = 0x80000000;
> >> size = 0x80000000;
> >>
> >> I expect it to fail ....
> >>
> >> With the formula you propose it will succeed, won't it ?
> >
> > Hmmm... Was i getting confused about some comments for 64bit
> > about there being such a big hole between valid user and kernel
> > addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
> >
> > That would be true for 64bit x86 (and probably ppc (& arm??))
> > if TASK_SIZE_MAX were 0x4 << 60.
> > IIUC the highest user address is (much) less than 0x0 << 60
> > and the lowest kernel address (much) greater than 0xf << 60
> > on all these 64bit platforms.
> >
> > Actually if doing access_ok() inside get_user() you don't
> > need to check the size at all.
> 
> You mean on 64 bit or on any platform ?

64bit and 32bit

> What about a word write to 0xbffffffe, won't it overwrite 0xc0000000 ?
> 
> > You don't even need to in copy_to/from_user() provided
> > it always does a forwards copy.
> 
> Do you mean due to the gap ?
> Is it garantied to be a gap ? Even on a 32 bits having TASK_SIZE set to
> 0xc0000000 and PAGE_OFFSET set to the same ?

I read somewhere (I won't find it again) that the last 4k page
(below 0xc0000000) must not be allocated on i386 because some
cpu (both intel and amd) do 'horrid things' if they try to
(IIRC) do instruction prefetches across the boundary.
So the accesses to 0xbffffffe will fault and the one to 0xc0000000
won't happen (in any useful way at least).

I'd suspect that not allocating the 3G-4k page would be a safe
bet on all architectures - even 68k.

	David

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

  reply	other threads:[~2020-09-02 15:40 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27 15:00 remove the last set_fs() in common code, and remove it for x86 and powerpc v2 Christoph Hellwig
2020-08-27 15:00 ` [PATCH 01/10] fs: don't allow kernel reads and writes without iter ops Christoph Hellwig
2020-08-27 15:58   ` David Laight
2020-08-29  9:23     ` 'Christoph Hellwig'
     [not found]   ` <20200901064849.GI4299@shao2-debian>
2020-09-01  7:08     ` [fs] ef30fb3c60: kernel write not supported for file /sys/kernel/softlockup_panic Christoph Hellwig
2020-08-27 15:00 ` [PATCH 02/10] fs: don't allow splice read/write without explicit ops Christoph Hellwig
2020-08-27 15:00 ` [PATCH 03/10] uaccess: add infrastructure for kernel builds with set_fs() Christoph Hellwig
2020-08-27 15:00 ` [PATCH 04/10] test_bitmap: skip user bitmap tests for !CONFIG_SET_FS Christoph Hellwig
2020-08-27 15:00 ` [PATCH 05/10] lkdtm: disable set_fs-based " Christoph Hellwig
2020-08-27 18:06   ` Linus Torvalds
2020-08-29  9:24     ` Christoph Hellwig
2020-09-01 18:52       ` Kees Cook
2020-09-01 18:57       ` Kees Cook
2020-09-02  8:09         ` Christoph Hellwig
2020-08-27 15:00 ` [PATCH 06/10] x86: move PAGE_OFFSET, TASK_SIZE & friends to page_{32,64}_types.h Christoph Hellwig
2020-08-27 15:00 ` [PATCH 07/10] x86: make TASK_SIZE_MAX usable from assembly code Christoph Hellwig
2020-08-27 15:00 ` [PATCH 08/10] x86: remove address space overrides using set_fs() Christoph Hellwig
2020-08-27 18:15   ` Linus Torvalds
2020-08-29  9:25     ` Christoph Hellwig
2020-08-27 15:00 ` [PATCH 09/10] powerpc: use non-set_fs based maccess routines Christoph Hellwig
2020-08-27 15:00 ` [PATCH 10/10] powerpc: remove address space overrides using set_fs() Christoph Hellwig
2020-09-02  6:15   ` Christophe Leroy
2020-09-02 12:36     ` Christoph Hellwig
2020-09-02 13:13       ` David Laight
2020-09-02 13:24         ` Christophe Leroy
2020-09-02 13:51           ` David Laight
2020-09-02 14:12             ` Christophe Leroy
2020-09-02 15:02               ` David Laight [this message]
2020-09-02 15:17       ` Christophe Leroy
2020-09-02 18:02         ` Linus Torvalds
2020-09-03  7:11           ` Christoph Hellwig
2020-09-03  7:27             ` Christophe Leroy
2020-09-03  8:55             ` Christophe Leroy
2020-09-03  7:20           ` Christophe Leroy
2020-08-27 15:31 ` remove the last set_fs() in common code, and remove it for x86 and powerpc v2 Christoph Hellwig
2020-09-01 17:13 ` Christophe Leroy
2020-09-01 17:25   ` Al Viro
2020-09-01 17:42     ` Matthew Wilcox
2020-09-01 18:39     ` Christophe Leroy
2020-09-01 19:01     ` Christophe Leroy
2020-09-02  8:10     ` Christoph Hellwig
2020-10-27  9:29 ` [PATCH 02/10] fs: don't allow splice read/write without explicit ops David Howells
2020-10-27  9:51 ` David Howells
2020-10-27  9:54   ` Christoph Hellwig
2020-10-27 10:38   ` David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1599b80426ec4759b5c1beb9d9543fdc@AcuMS.aculab.com \
    --to=david.laight@aculab.com \
    --cc=christophe.leroy@csgroup.eu \
    --cc=hch@lst.de \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=x86@kernel.org \
    --subject='RE: [PATCH 10/10] powerpc: remove address space overrides using set_fs()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).