Re: [PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type

[Leon Romanovsky <leon@kernel.org>]

On Tue, Dec 08, 2020 at 10:55:39AM +0300, Dan Carpenter wrote:
> On Tue, Dec 08, 2020 at 09:35:45AM +0200, Leon Romanovsky wrote:
> > @@ -336,19 +335,16 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
> >  		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
> >  		user_entry_size);
> >  	if (max_entries <= 0)
> > -		return -EINVAL;
> > +		return max_entries ?: -EINVAL;
> >
> >  	ucontext = ib_uverbs_get_ucontext(attrs);
> >  	if (IS_ERR(ucontext))
> >  		return PTR_ERR(ucontext);
> >  	ib_dev = ucontext->device;
> >
> > -	if (check_mul_overflow(max_entries, sizeof(*entries), &num_bytes))
> > -		return -EINVAL;
> > -
> > -	entries = uverbs_zalloc(attrs, num_bytes);
> > -	if (!entries)
> > -		return -ENOMEM;
> > +	entries = uverbs_kcalloc(attrs, max_entries, sizeof(*entries));
> > +	if (IS_ERR(entries))
> > +		return PTR_ERR(entries);
>
> This isn't right.  The uverbs_kcalloc() should match every other
> kcalloc() function and return NULL on error.  This actually buggy
> because it returns both is error pointers and NULL so it will lead to
> a NULL dereference.

The actual bug was before, when an error result from uverbs_zalloc()
was treated as NULL. The uverbs_kcalloc/uverbs_zalloc will call to
_uverbs_alloc() that doesn't return NULL.

>
> Btw, when a function returns both error pointers and NULL the NULL
> return means that the feature has been deliberately disabled.  It's not
> an error pointer because it's deliberate.
>
> regards,
> dan carpenter
>
> >
> >  	num_entries = rdma_query_gid_table(ib_dev, entries, max_entries);
> >  	if (num_entries < 0)
> > diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> > index bf167ef6c688..39ef204753ec 100644
> > --- a/include/rdma/uverbs_ioctl.h
> > +++ b/include/rdma/uverbs_ioctl.h
> > @@ -865,6 +865,16 @@ static inline __malloc void *uverbs_zalloc(struct uverbs_attr_bundle *bundle,
> >  {
> >  	return _uverbs_alloc(bundle, size, GFP_KERNEL | __GFP_ZERO);
> >  }
> > +
> > +static inline __malloc void *uverbs_kcalloc(struct uverbs_attr_bundle *bundle,
> > +					    size_t n, size_t size)
> > +{
> > +	size_t bytes;
> > +
> > +	if (unlikely(check_mul_overflow(n, size, &bytes)))
> > +		return ERR_PTR(-EOVERFLOW);
> > +	return uverbs_zalloc(bundle, bytes);
> > +}
> >  int _uverbs_get_const(s64 *to, const struct uverbs_attr_bundle *attrs_bundle,
> >  		      size_t idx, s64 lower_bound, u64 upper_bound,
> >  		      s64 *def_val);
> > --
> > 2.28.0