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.
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
