Messages in this thread | | | Date | Mon, 7 Nov 2022 20:33:52 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCHv11 05/16] x86/uaccess: Provide untagged_addr() and remove tags before address check |
| |
On Mon, Nov 07, 2022 at 06:50:51AM -0800, Andy Lutomirski wrote: > > @@ -21,6 +22,30 @@ static inline bool pagefault_disabled(void); > > # define WARN_ON_IN_IRQ() > > #endif > > +#ifdef CONFIG_X86_64 > > +/* > > + * Mask out tag bits from the address. > > + * > > + * Magic with the 'sign' allows to untag userspace pointer without any branches > > + * while leaving kernel addresses intact. > > + */ > > +#define untagged_addr(mm, addr) ({ \ > > + u64 __addr = (__force u64)(addr); \ > > + s64 sign = (s64)__addr >> 63; \ > > + __addr &= (mm)->context.untag_mask | sign; \ > > + (__force __typeof__(addr))__addr; \ > > +}) > > + > > I think this implementation is correct, but I'm wondering if there are any > callers of untagged_addr that actually need to preserve kernel addresses. > Are there? (There certainly *were* back when we had set_fs().)
I don't think there's any.
CONFIG_KASAN_SW_TAGS uses untagged_addr() on kernel addresses, but it is only enabled on arm64. On x86, it will use CR4.LAM_SUP and the enabling would require a new helper for untagging kernel addresses.
That said, I would rather stay on the safe side.
> I'm also mildly uneasy about a potential edge case. Naively, one would > expect: > > untagged_addr(current->mm, addr) + size == > untagged_addr(current->mm, addr + size) > > at least for an address that is valid enough to be potentially dereferenced. > This isn't true any more for size that overflows into the tag bit range.
That's definitely a new edge case.
From quick grep, the only CONFIG_KASAN_SW_TAGS code obviously does arithmetics on address before untagging.
> I *think* we're okay though -- __access_ok requires that addr <= limit - > size, so any range that overflows into tag bits will be rejected even if the > entire range consists of valid (tagged) user addresses.
True.
> So: > > Acked-by: Andy Lutomirski <luto@kernel.org>
Thanks!
-- Kiryl Shutsemau / Kirill A. Shutemov
| |