| Message ID | 20241115205206.17678-1-zichenxie0106@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: uaccess: Restrict user access to kernel memory in __copy_user_flushcache() | expand |
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. > 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. Will
[ 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.
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); /* See above */ dcache_clean_pop((unsigned long)to, (unsigned long)to + n - rc);