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

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Avihai Horon <avihaih@nvidia.com>,
	linux-rdma@vger.kernel.org
Subject: Re: [PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type
Date: Tue, 8 Dec 2020 10:55:39 +0300	[thread overview]
Message-ID: <20201208075539.GA2789@kadam> (raw)
In-Reply-To: <20201208073545.9723-4-leon@kernel.org>

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.

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