Re: [PATCHv3 5/8] x86/uaccess: Provide untagged_addr() and remove tags before address check

From: "Andy Lutomirski" <luto@kernel.org>
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: "Dave Hansen" <dave.hansen@linux.intel.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"kcc@google.com" <kcc@google.com>,
	"ryabinin.a.a@gmail.com" <ryabinin.a.a@gmail.com>,
	"andreyknvl@gmail.com" <andreyknvl@gmail.com>,
	"glider@google.com" <glider@google.com>,
	"dvyukov@google.com" <dvyukov@google.com>,
	"H.J. Lu" <hjl.tools@gmail.com>,
	"Andi Kleen" <ak@linux.intel.com>,
	"Rick P Edgecombe" <rick.p.edgecombe@intel.com>,
	linux-mm@kvack.org,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCHv3 5/8] x86/uaccess: Provide untagged_addr() and remove tags before address check
Date: Wed, 29 Jun 2022 19:38:20 -0700	[thread overview]
Message-ID: <7c8381b3-c71b-45e8-a162-c9701dabcc9b@www.fastmail.com> (raw)
In-Reply-To: <20220629004257.x3pyoydmtk2lhrnx@black.fi.intel.com>

On Tue, Jun 28, 2022, at 5:42 PM, Kirill A. Shutemov wrote:
> On Tue, Jun 28, 2022 at 04:40:48PM -0700, Andy Lutomirski wrote:
>> On 6/10/22 07:35, Kirill A. Shutemov wrote:
>> > untagged_addr() is a helper used by the core-mm to strip tag bits and
>> > get the address to the canonical shape. In only handles userspace
>> > addresses. The untagging mask is stored in mmu_context and will be set
>> > on enabling LAM for the process.
>> > 
>> > The tags must not be included into check whether it's okay to access the
>> > userspace address.
>> > 
>> > Strip tags in access_ok().
>> 
>> What is the intended behavior for an access that spans a tag boundary?
>
> You mean if 'addr' passed to access_ok() is below 47- or 56-bit but 'addr'
> + 'size' overflows into tags? If is not valid access and the current
> implementation works correctly.
>
>> Also, at the risk of a potentially silly question, why do we need to strip
>> the tag before access_ok()?  With LAM, every valid tagged user address is
>> also a valid untagged address, right?  (There is no particular need to
>> enforce the actual value of TASK_SIZE_MAX on *access*, just on mmap.)
>> 
>> IOW, wouldn't it be sufficient, and probably better than what we have now,
>> to just check that the entire range has the high bit clear?
>
> No. We do things to addresses on kernel side beyond dereferencing them.
> Like comparing addresses have to make sense. find_vma() has to find
> relevant mapping and so on. 

I think you’re misunderstanding me. Of course things like find_vma() need to untag the address. (But things like munmap, IMO, ought not accept tagged addresses.)

But I’m not talking about that at all. I’m asking why we need to untag an address for access_ok.  In the bad old days, access_ok checked that a range was below a *variable* cutoff. But set_fs() is gone, and I don’t think this is needed any more.

With some off-the-cuff bit hackery, I think it ought to be sufficient to calculate addr+len and fail if the overflow or sign bits get set. If LAM is off, we could calculate addr | len and fail if either of the top two bits is set, but LAM may not let us get away with that.  The general point being that, on x86 (as long as we ignore AMD’s LAM-like feature) an address is a user address if the top bit is clear. Whether that address is canonical or not or will translate or not is a separate issue. (And making this change would require allowing uaccess to #GP, which requires some care.)

What do you think?

>
> -- 
>  Kirill A. Shutemov