Re: [bug report] RDMA/uverbs: Introduce UCAP (User CAPabilities) API

[Leon Romanovsky <leon@kernel.org>]

On Wed, Mar 12, 2025 at 09:53:17AM +0300, Dan Carpenter wrote:
> Hello Chiara Meiohas,
> 
> Commit 61e51682816d ("RDMA/uverbs: Introduce UCAP (User CAPabilities)
> API") from Mar 6, 2025 (linux-next), leads to the following Smatch
> static checker warning:
> 
> 	drivers/infiniband/core/ucaps.c:209 ib_release_ucap()
> 	error: buffer overflow 'ucaps_list' 2 <= 2 (assuming for loop doesn't break)

The thing is that we must have "break", so writing if(WARN_ON(type ==
"RDMA_UCAP_MAX)) return;" instead of existing WARN_ON is very
misleading.

Thanks

> 
> drivers/infiniband/core/ucaps.c
>     198 static void ib_release_ucap(struct kref *ref)
>     199 {
>     200         struct ib_ucap *ucap = container_of(ref, struct ib_ucap, ref);
>     201         enum rdma_user_cap type;
>     202 
>     203         for (type = RDMA_UCAP_FIRST; type < RDMA_UCAP_MAX; type++) {
>     204                 if (ucaps_list[type] == ucap)
>     205                         break;
>     206         }
>     207         WARN_ON(type == RDMA_UCAP_MAX);
> 
> This prints a warning if we're out of bounds, but it doesn't handle the
> error.  This is called from kref_put() and with kref_put() this could
> actually be done in a different thread with a delay from when
> ib_remove_ucap() is called.  I wouldn't advise that for production systems
> but it's supposed to work.
> 
> So this code makes me quite nervous.
> 
>     208 
> --> 209         ucaps_list[type] = NULL;
>     210         cdev_device_del(&ucap->cdev, &ucap->dev);
>     211         put_device(&ucap->dev);
>     212 }
> 
> regards,
> dan carpenter
>