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

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

[Dan Carpenter]

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)

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

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

[Leon Romanovsky]

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
> 

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

[Dan Carpenter]

On Wed, Mar 12, 2025 at 10:07:17AM +0200, Leon Romanovsky wrote:
> 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.
> 

Ah, never mind.  I got confused between kref_put() and kobject_put().
It's the kobject_release() function that can optionally do the release in
another thread.  Sorry for the noise.

regards,
dan carpenter