| Message ID | a958e202cdbe6e1bac8a37b7f3d9881d1b22993d.1550839937.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: untag user pointers passed to the kernel | expand |
On 2/22/19 4:53 AM, Andrey Konovalov wrote: > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2730,7 +2730,7 @@ void *copy_mount_options(const void __user * data) > * the remainder of the page. > */ > /* copy_from_user cannot cross TASK_SIZE ! */ > - size = TASK_SIZE - (unsigned long)data; > + size = TASK_SIZE - (unsigned long)untagged_addr(data); > if (size > PAGE_SIZE) > size = PAGE_SIZE; I would have thought that copy_from_user() *is* entirely capable of detecting and returning an error in the case that its arguments cross TASK_SIZE. It will fail and return an error, but that's what it's supposed to do. I'd question why this code needs to be doing its own checking in the first place. Is there something subtle going on?
On Sat, Feb 23, 2019 at 12:03 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 2/22/19 4:53 AM, Andrey Konovalov wrote: > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -2730,7 +2730,7 @@ void *copy_mount_options(const void __user * data) > > * the remainder of the page. > > */ > > /* copy_from_user cannot cross TASK_SIZE ! */ > > - size = TASK_SIZE - (unsigned long)data; > > + size = TASK_SIZE - (unsigned long)untagged_addr(data); > > if (size > PAGE_SIZE) > > size = PAGE_SIZE; > > I would have thought that copy_from_user() *is* entirely capable of > detecting and returning an error in the case that its arguments cross > TASK_SIZE. It will fail and return an error, but that's what it's > supposed to do. > > I'd question why this code needs to be doing its own checking in the > first place. Is there something subtle going on? The comment above exact_copy_from_user() states: Some copy_from_user() implementations do not return the exact number of bytes remaining to copy on a fault. But copy_mount_options() requires that. Note that this function differs from copy_from_user() in that it will oops on bad values of `to', rather than returning a short copy.
diff --git a/fs/namespace.c b/fs/namespace.c index a677b59efd74..d4b7adef9204 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2730,7 +2730,7 @@ void *copy_mount_options(const void __user * data) * the remainder of the page. */ /* copy_from_user cannot cross TASK_SIZE ! */ - size = TASK_SIZE - (unsigned long)data; + size = TASK_SIZE - (unsigned long)untagged_addr(data); if (size > PAGE_SIZE) size = PAGE_SIZE;
In copy_mount_options a user address is being subtracted from TASK_SIZE. If the address is lower than TASK_SIZE, the size is calculated to not allow the exact_copy_from_user() call to cross TASK_SIZE boundary. However if the address is tagged, then the size will be calculated incorrectly. Untag the address before subtracting. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- fs/namespace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)