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
