On Tue, Apr 28, 2020 at 07:16:29PM +0100, Kevin Brodsky wrote:
> On 21/04/2020 15:26, Catalin Marinas wrote:
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index 32fc8061aa76..566da441eba2 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -416,6 +416,17 @@ extern unsigned long __must_check __arch_copy_in_user(void __user *to, const voi
> > #define INLINE_COPY_TO_USER
> > #define INLINE_COPY_FROM_USER
> > +static inline bool arch_has_exact_copy_from_user(unsigned long n)
> > +{
> > + /*
> > + * copy_from_user() aligns the source pointer if the size is greater
> > + * than 15. Since all the loads are naturally aligned, they can only
> > + * fail on the first byte.
> > + */
> > + return n > 15;
> > +}
> > +#define arch_has_exact_copy_from_user
> > +
> > extern unsigned long __must_check __arch_clear_user(void __user *to, unsigned long n);
> > static inline unsigned long __must_check __clear_user(void __user *to, unsigned long n)
> > {
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index a28e4db075ed..8febc50dfc5d 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -3025,13 +3025,16 @@ void *copy_mount_options(const void __user * data)
> > if (!copy)
> > return ERR_PTR(-ENOMEM);
> > - size = PAGE_SIZE - offset_in_page(data);
> > + size = PAGE_SIZE;
> > + if (!arch_has_exact_copy_from_user(size))
> > + size -= offset_in_page(data);
> > - if (copy_from_user(copy, data, size)) {
> > + if (copy_from_user(copy, data, size) == size) {
> > kfree(copy);
> > return ERR_PTR(-EFAULT);
> > }
> > if (size != PAGE_SIZE) {
> > + WARN_ON(1);
>
> I'm not sure I understand the rationale here. If we don't have exact
> copy_from_user for size, then we will attempt to copy up to the end of the
> page. Assuming this doesn't fault, we then want to carry on copying from the
> start of the next page, until we reach a total size of up to 4K. Why would
> we warn in that case?
We shouldn't warn, thanks for spotting this. I added it for some testing
and somehow ended up in the commit.
--
Catalin
