[patch] IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()

[patch] IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()

[Dan Carpenter]

We don't set "resp.reserved".  Since it's at the end of the struct that
means we don't have to copy it to the user.

Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 8000fff..43dfb84 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -619,7 +619,8 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
 
 	resp.tot_uuars = req.total_num_uuars;
 	resp.num_ports = dev->mdev.caps.num_ports;
-	err = ib_copy_to_udata(udata, &resp, sizeof(resp));
+	err = ib_copy_to_udata(udata, &resp,
+			       sizeof(resp) - sizeof(resp.reserved));
 	if (err)
 		goto out_uars;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()

[Eli Cohen]

On Thu, Jul 25, 2013 at 08:04:36PM +0300, Dan Carpenter wrote:
> We don't set "resp.reserved".  Since it's at the end of the struct that
> means we don't have to copy it to the user.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 8000fff..43dfb84 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -619,7 +619,8 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
>  
>  	resp.tot_uuars = req.total_num_uuars;
>  	resp.num_ports = dev->mdev.caps.num_ports;
> -	err = ib_copy_to_udata(udata, &resp, sizeof(resp));
> +	err = ib_copy_to_udata(udata, &resp,
> +			       sizeof(resp) - sizeof(resp.reserved));
>  	if (err)
>  		goto out_uars;
>

I don't have strong opinion on this one. The title of the patch is
stack info leak but the only leak of a reserved field. Other opinions?

Re: [patch] IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()

[Dan Carpenter]

On Sun, Jul 28, 2013 at 10:23:36AM +0300, Eli Cohen wrote:
> On Thu, Jul 25, 2013 at 08:04:36PM +0300, Dan Carpenter wrote:
> > We don't set "resp.reserved".  Since it's at the end of the struct that
> > means we don't have to copy it to the user.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> > index 8000fff..43dfb84 100644
> > --- a/drivers/infiniband/hw/mlx5/main.c
> > +++ b/drivers/infiniband/hw/mlx5/main.c
> > @@ -619,7 +619,8 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
> >  
> >  	resp.tot_uuars = req.total_num_uuars;
> >  	resp.num_ports = dev->mdev.caps.num_ports;
> > -	err = ib_copy_to_udata(udata, &resp, sizeof(resp));
> > +	err = ib_copy_to_udata(udata, &resp,
> > +			       sizeof(resp) - sizeof(resp.reserved));
> >  	if (err)
> >  		goto out_uars;
> >
> 
> I don't have strong opinion on this one. The title of the patch is
> stack info leak but the only leak of a reserved field. Other opinions?

First let me say that I don't know how this code is called, it may
be root only, but even in that case I think it's still worth
applying my patch.

These info leak problems are a well known security problem so I
didn't put a long explanation.  What you do is you fill the stack
with function pointers, then you call the function that leaks.  Then
you have a potentially useful pointer which was supposed to be
secret.  Something like that anyway.

There are probably lots of other easier ways to defeat address space
randomization.  There may be other ways you can use info leaks as
well...

Anyway, regardless, static checkers and code auditors look for these
leaks so applying the patch makes sense just to silence a warning.

regards,
dan carpenter

Re: [patch] IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()

[Eli Cohen]

On Sun, Jul 28, 2013 at 11:24:43PM +0300, Dan Carpenter wrote:
> 
> First let me say that I don't know how this code is called, it may
> be root only, but even in that case I think it's still worth
> applying my patch.

It can be called by non root users as well.

> 
> These info leak problems are a well known security problem so I
> didn't put a long explanation.  What you do is you fill the stack
> with function pointers, then you call the function that leaks.  Then
> you have a potentially useful pointer which was supposed to be
> secret.  Something like that anyway.
> 
> There are probably lots of other easier ways to defeat address space
> randomization.  There may be other ways you can use info leaks as
> well...
> 
> Anyway, regardless, static checkers and code auditors look for these
> leaks so applying the patch makes sense just to silence a warning.
> 

OK, I am convinced that it's worth applying.

Acked by Eli Cohen <eli@mellanox.com>