Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Christoph Hellwig' <hch@lst.de>,
        Miles Chen <miles.chen@mediatek.com>
CC:     "David S . Miller" <davem@davemloft.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        "wsd_upstream@mediatek.com" <wsd_upstream@mediatek.com>
Subject: RE: [PATCH] net: untag pointer in sockptr_is_kernel
Thread-Topic: [PATCH] net: untag pointer in sockptr_is_kernel
Thread-Index: AQHWb9DKxxLX2AshVECOBLD3J//Za6kyxXFQ
Date:   Tue, 11 Aug 2020 11:44:59 +0000
Message-ID: <36e381c558e24185bc2f7e80a758d06a@AcuMS.aculab.com>
References: <20200811102704.17875-1-miles.chen@mediatek.com>
 <20200811111551.GA3958@lst.de>
In-Reply-To: <20200811111551.GA3958@lst.de>
Accept-Language: en-GB, en-US
Content-Language: en-US
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> On Tue, Aug 11, 2020 at 06:27:04PM +0800, Miles Chen wrote:
> > From: Miles Chen <miles.chen@mediatek.com>
> >
> > sockptr_is_kernel() uses (sockptr.kernel >= TASK_SIZE) to tell
> > if the pointer is kernel space or user space. When user space uses
> > the "top byte ignored" feature such as HWAsan, we must untag
> > the pointer before checking against TASK_SIZE.
> >
> > sockptr_is_kernel() will view a tagged user pointer as a kernel pointer
> > and use memcpy directly and causes a kernel crash.
> 
> Dave merged a patch from me to rever the optimized sockptr
> implementation for now.  If we bring it back we should fold in your
> fix.

I missed that going though :-(
I've looked for a fix to the access_ok(kernel_addr,0) being true issue.

Shouldn't TASK_SIZE be increased to cover all the 'tagged' addresses?
ISTR the 'tag' bits are the 'next' 8 (or so) address bits at the top
of valid user addresses.

Then presumably the user-copies would be able to use the tagged
address values getting whatever protection that gives.

ISTM that for kernel-user boundary checks TASK_SIZE is the
wrong constant.
(The upper limit for mmap() is entirely different.)
The limit should be independent of whether the process is 32 or 64bit
(any fault above 4G will fail to find a user-page for 32bit).
The limit can also go well into the address 'black hole' that
exists on x86-x64 (and similar) between valid user and kernel
addresses - handling the relevant trap should be too hard
(it is always an error, so need not be fast).

So with set_fs(KERNEL_DS) gone x86-x64 can (almost certainly)
do a cheap test for (long)addr >= 0 in access_ok() (+ length test).
While set_fs() is needed it can be:
	((long)addr & current->mask) >= 0
(masking off the top bit if kernel addresses are valid).

	David

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