IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()
Subsystems (3)
PATCH 1/1 IB/mlx5: stack info leak in mlx5_ib_alloc_ucontext()
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.
drivers/infiniband/hw/mlx5/main.c +2 -1
@@ -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;
Quoted 10 lines · 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 <[email protected]>drivers/infiniband/hw/mlx5/main.c +0
-0
drivers/infiniband/hw/mlx5/main.c static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev, · line 626
Quoted 9 lines
drivers/infiniband/hw/mlx5/main.c +2
-1@@ -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?
Quoted 12 lines · Eli Cohen
Quoted 11 lines · 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 <[email protected]>drivers/infiniband/hw/mlx5/main.c +0
-0@@ -619,7 +619,8 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
Quoted 11 lines
drivers/infiniband/hw/mlx5/main.c static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev, · line 626
Quoted 8 lines
drivers/infiniband/hw/mlx5/main.c +2
-1resp.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
Quoted 4 lines · Dan Carpenter
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.
Quoted 7 lines
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.
Quoted 7 lines
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 <[email protected]>