On Wed, Mar 28, 2018 at 11:02:03AM -0600, Jason Gunthorpe wrote:
> On Wed, Mar 28, 2018 at 07:43:06AM +0300, Leon Romanovsky wrote:
> > On Tue, Mar 27, 2018 at 10:32:23PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Mar 28, 2018 at 06:33:30AM +0300, Leon Romanovsky wrote:
> > > > On Tue, Mar 27, 2018 at 01:45:40PM -0600, Jason Gunthorpe wrote:
> > > > > On Sun, Mar 25, 2018 at 11:10:27AM +0300, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > >
> > > > > > Ensure that user provides valid AF family prior to query device address.
> > > > > >
> > > > > > Reported-by: <syzbot+2a2c48fc189ed5125b9c@xxxxxxxxxxxxxxxxxxxxxxxxx>
> > > > > > Fixes: ee7aed4528fb ("RDMA/ucma: Support querying for AF_IB addresses")
> > > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > > drivers/infiniband/core/ucma.c | 7 +++++--
> > > > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
> > > > > > index 5db68d86070f..2178022badcc 100644
> > > > > > +++ b/drivers/infiniband/core/ucma.c
> > > > > > @@ -879,10 +879,13 @@ static ssize_t ucma_query_addr(struct ucma_context *ctx,
> > > > > >
> > > > > > addr = (struct sockaddr *) &ctx->cm_id->route.addr.src_addr;
> > > > > > resp.src_size = rdma_addr_size(addr);
> > > > > > - memcpy(&resp.src_addr, addr, resp.src_size);
> > > > > > -
> > > > > > addr = (struct sockaddr *) &ctx->cm_id->route.addr.dst_addr;
> > > > > > resp.dst_size = rdma_addr_size(addr);
> > > > > > +
> > > > > > + if (!resp.src_size || !resp.dst_size)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + memcpy(&resp.src_addr, addr, resp.src_size);
> > > > > > memcpy(&resp.dst_addr, addr, resp.dst_size);
> > > > >
> > > > > Woah, no, NAK! Moving the memcpy without revising addr breaks it.
> > > >
> > > > Ahh, right
> > > >
> > > > >
> > > > > .. and I don't understand this at all, how does 0 src_size cause a
> > > > > null pointer deref?
> > > > >
> > > > > memcpy(a,b,0) is OK right?
> > > >
> > > > In C it is ok, but KASAN complains about it.
> > >
> > > That makes no sense.
> > >
> > > The code in the syzkaller oops disassembles the faulting instruction
> > > to
> > >
> > > movzbl (%rdx,%rax,1),%edx
> > > rdx = 0000000000000018
> > > rax = ffffffffffffffda
> > >
> > > Which to me looks rather a lot like an err_ptr.. And nothing like a memcpy..
> > >
> > > Since syzkaller says it is reproducing that without e8980d67d60 which
> > > prevents err_ptr in cm_id, I wonder if this is just a dup?
> >
> > It can, however it is less important, at least for me.
> >
> > I don't see any reason why we would want to skip check of user-input and
> > rely on the current code which doesn't use those fields. It seems too
> > fragile to me.
>
> ???
>
> rdma_addr_size(&ctx->cm_id->route.addr.dst_addr) is not user
> input, it is part of the kernel state at this point. Right?
>
> Can rdma_addr_size even return 0 at this point?
>
> If yes, then we should return EINVAL, but that is to make the API sane
> for the user not to protect the kernel.
I'm not near code now, but from what I remember, the user can call to
rdma_create_id(), it will create new ctx->cm_id but with addr zeroed,
because ucma_alloc_ctx() uses kzalloc.
After that this user will call it ucma_query() and will hit this flow.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
