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.
>
> > 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.
>
> @@ -642,12 +642,12 @@ static void mlx5_ib_unlock_cqs(struct mlx5_ib_cq *send_cq,
> struct mlx5_ib_cq *recv_cq);
>
> int bfregn_to_uar_index(struct mlx5_ib_dev *dev,
> - struct mlx5_bfreg_info *bfregi, int bfregn,
> + struct mlx5_bfreg_info *bfregi, u32 bfregn,
> bool dyn_bfreg)
> {
> - int bfregs_per_sys_page;
> - int index_of_sys_page;
> - int offset;
> + unsigned int bfregs_per_sys_page;
> + u32 index_of_sys_page;
> + u32 offset;
>
> bfregs_per_sys_page = get_uars_per_sys_page(dev, bfregi->lib_uar_4k) *
> MLX5_NON_FP_BFREGS_PER_UAR;
>
> 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
