[ 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > 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.
