On Fri, Dec 07, 2018 at 08:26:10PM +0000, Arumugam, Kamenee wrote:
> Subject: Re: [PATCH for-next v2 2/4] IB/hfi1: Move rvt_cq_wc struct into uapi directory
>
> On Wed, Dec 05, 2018 at 07:10:52AM -0800, Dennis Dalessandro wrote:
> > +#include <linux/types.h>
> > +#include <rdma/ib_user_verbs.h>
> > +#ifndef RDMA_ATOMIC_UAPI
> > +#define RDMA_ATOMIC_UAPI(_type, _name) _type _name #endif
> > +/*
> > + * This structure is used to contain the head pointer, tail pointer,
> > + * and completion queue entries as a single memory allocation so
> > + * it can be mmap'ed into user space.
> > + */
> > +struct rvt_cq_wc {
> > + /* index of next entry to fill */
> > + RDMA_ATOMIC_UAPI(u32, head);
> > + /* index of next ib_poll_cq() entry */
> > + RDMA_ATOMIC_UAPI(u32, tail);
>
> > I went and looked at all the readers of these variables, and they are all missing a READ_ONCE (ie the kernel version of
> > atomic_load(relaxed))
>
> > I think you should change the definition to be:
>
> > #define RDMA_ATOMIC_UAPI(_type, _name) struct {_type atomic_val;} _name
>
> > And then add a kernel function
>
> > #define RDMA_READ_UAPI_ATOMIC(_member) READ_ONCE((_memb)->atomic_val)
>
> > So that the compiler protects against this mistake.
>
> There is already a producer and consumer lock for the circular
> buffer and only reading the opposition index needs the READ_ONCE
> control.
No.
Any read of memory mmap'd into user space must use READ_ONCE or risk a
security problem due to control-flow execution corruption.
Locks don't extend to untrusted userspace so they are useless for
this.
> https://www.kernel.org/doc/Documentation/circular-buffers.txt
This document is not considering the impact of mmaping the memory to
user space.
If you want to avoid the READ_ONCE then you have to maintain a shadow
value in secure kernel memory.
Same story for WRITE_ONCE.
This is why my suggestion to force the compiler to check this is
required to get it right.
Jason
