Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: ieff94742:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-713-g1f035dc716-fm-20220617.001-g1f035dc7
Mime-Version: 1.0
Message-Id: <7c8381b3-c71b-45e8-a162-c9701dabcc9b@www.fastmail.com>
In-Reply-To: <20220629004257.x3pyoydmtk2lhrnx@black.fi.intel.com>
References: <20220610143527.22974-1-kirill.shutemov@linux.intel.com>
 <20220610143527.22974-6-kirill.shutemov@linux.intel.com>
 <53d41f54-28ad-023c-537f-281cc2c40ae9@kernel.org>
 <20220629004257.x3pyoydmtk2lhrnx@black.fi.intel.com>
Date:   Wed, 29 Jun 2022 19:38:20 -0700
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
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


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 a=
nd
>> > 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.
>> >=20
>> > The tags must not be included into check whether it's okay to acces=
s the
>> > userspace address.
>> >=20
>> > Strip tags in access_ok().
>>=20
>> 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 'a=
ddr'
> + '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 addres=
s 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.)
>>=20
>> 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.=20

I think you=E2=80=99re misunderstanding me. Of course things like find_v=
ma() need to untag the address. (But things like munmap, IMO, ought not =
accept tagged addresses.)

But I=E2=80=99m not talking about that at all. I=E2=80=99m 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 gon=
e, and I don=E2=80=99t 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 LA=
M is off, we could calculate addr | len and fail if either of the top tw=
o bits is set, but LAM may not let us get away with that.  The general p=
oint being that, on x86 (as long as we ignore AMD=E2=80=99s LAM-like fea=
ture) 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 issu=
e. (And making this change would require allowing uaccess to #GP, which =
requires some care.)

What do you think?

>
> --=20
>  Kirill A. Shutemov