On Wed, Jun 27, 2018 at 10:35:43PM -0600, Jason Gunthorpe wrote:
> On Thu, Jun 28, 2018 at 07:24:56AM +0300, Leon Romanovsky wrote:
> > On Wed, Jun 27, 2018 at 01:49:44PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Jun 27, 2018 at 09:27:37PM +0300, Leon Romanovsky wrote:
> > > > > > index 5471b57b873d..6b0034c063f4 100644
> > > > > > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > > > > > @@ -653,6 +653,9 @@ int bfregn_to_uar_index(struct mlx5_ib_dev *dev,
> > > > > > int index_of_sys_page;
> > > > > > int offset;
> > > > > >
> > > > > > + if (bfregn > MLX5_MAX_BFREGS)
> > > > > > + return -EINVAL;
> > > > >
> > > > > This is not enough, you should make 'int bfregn' into 'unsigned int
> > > > > bfregn'.
> > > > >
> > > > > Because here we do:
> > > > > uar_index = bfregn_to_uar_index(dev, &context->bfregi,
> > > > > ucmd.bfreg_index, true);
> > > > >
> > > > > And
> > > > > __u32 bfreg_index;
> > > > >
> > > > > So, userspace could pass in 1<<31 and still pass (bfregn > MLX5_MAX_BFREGS)
> > > > > after all the casting is worked out.
> > > > >
> > > >
> > > > How? MLX5_MAX_BFREGS is 1 << 9 in mlx5
> > >
> > > Beacuse this prints success?
> > >
> > > void func(int arg)
> > > {
> > > if (arg > 9)
> > > return;
> > >
> > > printf("Success! %d\n", arg);
> > > }
> > >
> > > int main(int argc, const char *argv[])
> > > {
> > > uint32_t arg = -1;
> > >
> > > func(arg);
> > > }
> > >
> >
> > And what?
> > This function will also success
> > void func(void)
> > {
> > if (10 > 9)
> > return;
> >
> > printf("Success! %d\n", arg);
> > }
> >
> > But it is completely disconnected from the reality.
> > The bfregn_to_uar_index() function is called twice, one time with proper
> > values and another with something from the user.
>
> You asked why checking for a positive value was not sufficient, the
> above is why as the user can bypass the existing and proposed
> tests. Checking an int derived from user data for positive is not
> enough to safely bound it.
>
> > > > only negative value slipped through the cracks and passed the check
> > > > mentioned in commit message.
> > >
> > > I didn't understand that part of the commit message either. Aren't the
> > > existing tests just fine if you make the argument and other variables
> > > unsigned:
> > >
> > > index_of_sys_page = bfregn / bfregs_per_sys_page;
> > > if (index_of_sys_page >= bfregi->num_sys_pages)
> > > return -EINVAL;
> > >
> > > ??
> > >
> > > The issue is nothing is checking for negative values.
> > >
> > > That is solved correctly by not using signed types - not by adding
> > > more range checking.
> > >
> > > So this is the fix, as it seems the existing 'index_of_sys_page >=
> > > num_sys_pages' restriction is a perfectly fine array bound??a
> >
> > You need to change whole create_user_qp() function do not use bregn
> > as an output from alloc_bfreg, because that function returns < 0 for
> > errors. A lot of churn with low value.
>
> Why would that be needed? bfregn can remain int inside the function
> and since it is already prooven to be positive the implicit cast to
> unsigned will not truncate, while the user controlled u32 will be
> passed without mangling to the bounding check.
Because explicit is always better than implicit, and it is wrong to
leave in the code place which relies on that.
Thanks
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
