Re: [PATCH 05/18] Add io_uring IO interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/28/19 6:07 PM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>> The submission queue (SQ) and completion queue (CQ) rings are shared
>> between the application and the kernel. This eliminates the need to
>> copy data back and forth to submit and complete IO.
> [...]
>> +static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
>> +{
>> +       struct io_sq_ring *ring = ctx->sq_ring;
>> +       unsigned head;
>> +
>> +       /*
>> +        * The cached sq head (or cq tail) serves two purposes:
>> +        *
>> +        * 1) allows us to batch the cost of updating the user visible
>> +        *    head updates.
>> +        * 2) allows the kernel side to track the head on its own, even
>> +        *    though the application is the one updating it.
>> +        */
>> +       head = ctx->cached_sq_head;
>> +       smp_rmb();
>> +       if (head == READ_ONCE(ring->r.tail))
>> +               return false;
>> +
>> +       head = ring->array[head & ctx->sq_mask];
>> +       if (head < ctx->sq_entries) {
>> +               s->index = head;
>> +               s->sqe = &ctx->sq_sqes[head];
> 
> ring->array can be mapped writable into userspace, right? If so: This
> looks like a double-read issue; the compiler might assume that
> ring->array is not modified concurrently and perform separate memory
> accesses for the "if (head < ctx->sq_entries)" check and the
> "&ctx->sq_sqes[head]" computation. Please use READ_ONCE()/WRITE_ONCE()
> for all accesses to memory that userspace could concurrently modify in
> a malicious way.
> 
> There have been some pretty severe security bugs caused by missing
> READ_ONCE() annotations around accesses to shared memory; see, for
> example, https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf
> . Slides 35-48 show how the code "switch (op->cmd)", where "op" is a
> pointer to shared memory, allowed an attacker to break out of a Xen
> virtual machine because the compiler generated multiple memory
> accesses.

Thanks, I'll update these to use READ/WRITE_ONCE.

>> +               ctx->cached_sq_head++;
>> +               return true;
>> +       }
>> +
>> +       /* drop invalid entries */
>> +       ctx->cached_sq_head++;
>> +       ring->dropped++;
>> +       smp_wmb();
>> +       return false;
>> +}
> [...]
>> +SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
>> +               u32, min_complete, u32, flags, const sigset_t __user *, sig,
>> +               size_t, sigsz)
>> +{
>> +       struct io_ring_ctx *ctx;
>> +       long ret = -EBADF;
>> +       struct fd f;
>> +
>> +       f = fdget(fd);
>> +       if (!f.file)
>> +               return -EBADF;
>> +
>> +       ret = -EOPNOTSUPP;
>> +       if (f.file->f_op != &io_uring_fops)
>> +               goto out_fput;
> 
> Oh, by the way: If you feel like it, maybe you could add a helper
> fdget_typed(int fd, struct file_operations *f_op), or something like
> that, so that there is less boilerplate code for first doing fdget(),
> then checking ->f_op, and then coding an extra bailout path for that?
> But that doesn't really have much to do with your patchset, feel free
> to ignore this comment.

That's not a bad idea, I think this is a fairly common code pattern.
I'll look into it.

-- 
Jens Axboe




[Index of Archives]     [Kernel Documentation]     [Netdev]     [Linux Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux