On Wed, Sep 25, 2019 at 10:48:31AM -0700, Linus Torvalds wrote:
> On Wed, Sep 25, 2019 at 10:21 AM Aleksa Sarai <cyphar@xxxxxxxxxx> wrote:
> >
> > Just to make sure I understand, the following diff would this solve the
> > problem? If so, I'll apply it, and re-send in a few hours.
>
> Actually, looking at it more, it's still buggy.
>
> That final "size smaller than unsigned long" doesn't correctly handle
> the case of (say) a single byte in the middle of a 8-byte word.
>
> So you need to do something like this:
>
> int is_zeroed_user(const void __user *from, size_t size)
> {
> unsigned long val, mask, align;
>
> if (unlikely(!size))
> return true;
>
> if (!user_access_begin(from, size))
> return -EFAULT;
>
> align = (uintptr_t) from % sizeof(unsigned long);
> from -= align;
> size += align;
>
> mask = ~aligned_byte_mask(align);
>
> while (size >= sizeof(unsigned long)) {
> unsafe_get_user(val, (unsigned long __user *) from, err_fault);
> val &= mask;
> if (unlikely(val))
> goto done;
> mask = ~0ul;
> from += sizeof(unsigned long);
> size -= sizeof(unsigned long);
> }
>
> if (size) {
> /* (@from + @size) is unaligned. */
> unsafe_get_user(val, (unsigned long __user *) from, err_fault);
> mask &= aligned_byte_mask(size);
> val &= mask;
> }
IMO it's better to lift reading the first word out of the loop, like this:
align = (uintptr_t) from % sizeof(unsigned long);
from -= align;
unsafe_get_user(val, (unsigned long __user *) from, err_fault);
if (align) {
size += align;
val &= ~aligned_byte_mask(align);
}
while (size > sizeof(unsigned long)) {
if (unlikely(val))
goto done;
from += sizeof(unsigned long);
size -= sizeof(unsigned long);
unsafe_get_user(val, (unsigned long __user *) from, err_fault);
}
if (size != size(unsigned long))
val &= aligned_byte_mask(size);
done:
Do you see any problems with that variant?
