On Wed, May 29, 2019 at 12:28 AM David Howells [off-list ref] wrote:

Jann Horn [off-list ref] wrote:

quoted

I don't see you setting any special properties on the VMA that would
prevent userspace from extending its size via mremap() - no
VM_DONTEXPAND or VM_PFNMAP. So I think you might get an out-of-bounds
access here?

Should I just set VM_DONTEXPAND in watch_queue_mmap()?  Like so:

        vma->vm_flags |= VM_DONTEXPAND;

Yeah, that should work.

quoted

quoted

+static void watch_queue_map_pages(struct vm_fault *vmf,
+                                 pgoff_t start_pgoff, pgoff_t end_pgoff)

...

Same as above.

Same solution as above?  Or do I need ot check start/end_pgoff too?

Same solution.

quoted

quoted

+static int watch_queue_mmap(struct file *file, struct vm_area_struct *vma)
+{
+       struct watch_queue *wqueue = file->private_data;
+
+       if (vma->vm_pgoff != 0 ||
+           vma->vm_end - vma->vm_start > wqueue->nr_pages * PAGE_SIZE ||
+           !(pgprot_val(vma->vm_page_prot) & pgprot_val(PAGE_SHARED)))
+               return -EINVAL;

This thing should probably have locking against concurrent
watch_queue_set_size()?

Yeah.

        static int watch_queue_mmap(struct file *file,
                                    struct vm_area_struct *vma)
        {
                struct watch_queue *wqueue = file->private_data;
                struct inode *inode = file_inode(file);
                u8 nr_pages;

                inode_lock(inode);
                nr_pages = wqueue->nr_pages;
                inode_unlock(inode);

                if (nr_pages == 0 ||
                ...
                        return -EINVAL;

Looks reasonable.

quoted

quoted

+       smp_store_release(&buf->meta.head, len);

Why is this an smp_store_release()? The entire buffer should not be visible to
userspace before this setup is complete, right?

Yes - if I put the above locking in the mmap handler.  OTOH, it's a one-off
barrier...

quoted

quoted

+               if (wqueue->buffer)
+                       return -EBUSY;

The preceding check occurs without any locks held and therefore has no
reliable effect. It should probably be moved below the
inode_lock(...).

Yeah, it can race.  I'll move it into watch_queue_set_size().

Sounds good.

quoted

quoted

+static void free_watch(struct rcu_head *rcu)
+{
+       struct watch *watch = container_of(rcu, struct watch, rcu);
+
+       put_watch_queue(rcu_access_pointer(watch->queue));

This should be rcu_dereference_protected(..., 1).

That shouldn't be necessary.  rcu_access_pointer()'s description says:

 * It is also permissible to use rcu_access_pointer() when read-side
 * access to the pointer was removed at least one grace period ago, as
 * is the case in the context of the RCU callback that is freeing up
 * the data, ...

It's in an rcu callback function, so accessing the __rcu pointers in the RCU'd
struct should be fine with rcu_access_pointer().

Aah, whoops, you're right, I missed that paragraph in the
documentation of rcu_access_pointer().

quoted

quoted

+       /* We don't need the watch list lock for the next bit as RCU is
+        * protecting everything from being deallocated.

Does "everything" mean "the wqueue" or more than that?

Actually, just 'wqueue' and its buffer.  'watch' is held by us once we've
dequeued it as we now own the ref 'wlist' had on it.  'wlist' and 'wq' must be
pinned by the caller.

quoted

quoted

+                       if (release) {
+                               if (wlist->release_watch) {
+                                       rcu_read_unlock();
+                                       /* This might need to call dput(), so
+                                        * we have to drop all the locks.
+                                        */
+                                       wlist->release_watch(wlist, watch);

How are you holding a reference to `wlist` here? You got the reference through
rcu_dereference(), you've dropped the RCU read lock, and I don't see anything
that stabilizes the reference.

The watch record must hold a ref on the watched object if the watch_list has a
->release_watch() method.  In the code snippet above, the watch record now
belongs to us because we unlinked it under the wlist->lock some lines prior.

Ah, of course.

However, you raise a good point, and I think the thing to do is to cache
->release_watch from it and not pass wlist into (*release_watch)().  We don't
need to concern ourselves with cleaning up *wlist as it will be cleaned up
when the target object is removed.

Keyrings don't have a ->release_watch method and neither does the block-layer
notification stuff.

quoted

quoted

+       if (wqueue->pages && wqueue->pages[0])
+               WARN_ON(page_ref_count(wqueue->pages[0]) != 1);

Is there a reason why there couldn't still be references to the pages
from get_user_pages()/get_user_pages_fast()?

I'm not sure.  I'm not sure what to do if there are.  What do you suggest?

I would use put_page() instead of manually freeing it; I think that
should be enough? I'm not entirely sure though.

quoted

quoted

+       n->info &= (WATCH_INFO_LENGTH | WATCH_INFO_TYPE_FLAGS | WATCH_INFO_ID);

Should the non-atomic modification of n->info

n's an unpublished copy of some userspace data that's local to the function
instance.  There shouldn't be any way to race with it at this point.

Ah, derp. Yes.