Re: [PATCH] arm64: uaccess: Restrict user access to kernel memory in __copy_user_flushcache()

[Mark Rutland <mark.rutland@arm.com>]

[ adding uaccess / iov_iter / pmem folk, question at the end ]

On Mon, Nov 18, 2024 at 11:56:55AM +0000, Will Deacon wrote:
> On Fri, Nov 15, 2024 at 02:52:07PM -0600, Gax-c wrote:
> > From: Zichen Xie <zichenxie0106@gmail.com>
> > 
> > raw_copy_from_user() do not call access_ok(), so this code allowed
> > userspace to access any virtual memory address. Change it to
> > copy_from_user().
> 
> How can you access *any* virtual memory address, given that we force the
> address to map userspace via __uaccess_mask_ptr()?
> 
> > Fixes: 9e94fdade4d8 ("arm64: uaccess: simplify __copy_user_flushcache()")
> 
> I don't think that commit changed the semantics of the code, so if it's
> broken then I think it was broken before that change as well.

AFAICT we've never had an access_ok() in __copy_user_flushcache() or
__copy_from_user_flushcache() (which is the only caller of
__copy_user_flushcache()).

We could fold the two together to make that aspect slightly clearer;
IIUC we only had this out-of-line due ot the PAN toggling that we used
to have.

> > Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/arm64/lib/uaccess_flushcache.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/lib/uaccess_flushcache.c b/arch/arm64/lib/uaccess_flushcache.c
> > index 7510d1a23124..fb138a3934db 100644
> > --- a/arch/arm64/lib/uaccess_flushcache.c
> > +++ b/arch/arm64/lib/uaccess_flushcache.c
> > @@ -24,7 +24,7 @@ unsigned long __copy_user_flushcache(void *to, const void __user *from,
> >  {
> >  	unsigned long rc;
> >  
> > -	rc = raw_copy_from_user(to, from, n);
> > +	rc = copy_from_user(to, from, n);
> 
> Does anybody actually call this with an unchecked user address?
> 
> From a quick look, there are two callers of _copy_from_iter_flushcache():
> 
>   1. pmem_recovery_write() - looks like it's using a kernel address?
> 
>   2. dax_copy_from_iter() - has a comment saying the address was already
>                             checked in vfs_write().
> 
> What am I missing? It also looks like x86 elides the check.

IIUC the intent is that __copy_from_user_flushcache() is akin to
raw_copy_from_user(), and requires that the caller has already checked
access_ok(). The addition of __copy_from_user_flushcache() conicided
with __copy_from_user() being replaced with raw_copy_from_user(), and I
suspect the naming divergence was accidental.

That said, plain copy_from_user_iter() has an access_ok() check while
copy_from_user_iter_flushcache() doesn't (and it lakcs any explanatory
comment), so even if that's ok for current callers it seems like it
might be fragile.

So the real question is where is the access_ok() call intended to live?
I don't think it's meant to be in __copy_from_user_flushcache(), and is
intended to live in *some* caller, but it seems odd that
copy_from_user_iter() and copy_from_user_iter_flushcache() diverge.

Mark.