Re: [PATCH][next] dma-buf: Fix NULL pointer dereference in dma_fence_enable_sw_signaling()

[Christian König]

Am 11.10.23 um 18:13 schrieb Gustavo A. R. Silva:
> On 10/11/23 10:03, Kees Cook wrote:
> > On Wed, Oct 11, 2023 at 08:03:43AM -0600, Gustavo A. R. Silva wrote:
> > > Currently, a NULL pointer dereference will happen in function
> > > `dma_fence_enable_sw_signaling()` (at line 615), in case `chain`
> > > is not allocated in `mock_chain()` and this function returns
> > > `NULL` (at line 86). See below:
> > >
> > > drivers/dma-buf/st-dma-fence-chain.c:
> > >   86         chain = mock_chain(NULL, f, 1);
> > >   87         if (!chain)
> > >   88                 err = -ENOMEM;
> > >   89
> > >   90         dma_fence_enable_sw_signaling(chain);
> >
> > Instead of the larger patch, should line 88 here just do a "return
> > -ENOMEM" instead?
>
> Nope. I would have to add a `goto` to skip 
> `dma_fence_enable_sw_signaling(chain)`.
>
> I originally thought of that, but as other _signaling() functions have
> sanity-checks inside, I decided to go with that solution.
>
> This bug has been there since Sep 2022. So, adding a sanity check 
> inside that
> function should prevent any other issue of this same kind to enter the 
> codebase
> and stay there for years.

I'm trying to remove those sanity checks for years since they are hiding 
problems instead of getting them fixed.

Calling dma_fence_enable_sw_signaling with a NULL pointer is a coding 
error and not a recoverable runtime error.

The test case should be fixed instead.

Regards,
Christian.

> --
> Gustavo
>
> > -Kees
> >
> > > drivers/dma-buf/dma-fence.c:
> > >   611 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
> > >   612 {
> > >   613         unsigned long flags;
> > >   614
> > >   615         spin_lock_irqsave(fence->lock, flags);
> > >                    ^^^^^^^^^^^
> > >                     |
> > >               NULL pointer reference
> > >               if fence == NULL
> > >
> > >   616         __dma_fence_enable_signaling(fence);
> > >   617         spin_unlock_irqrestore(fence->lock, flags);
> > >   618 }
> > >
> > > Fix this by adding a NULL check before dereferencing `fence` in
> > > `dma_fence_enable_sw_signaling()`. This will prevent any other NULL
> > > pointer dereference when the `fence` passed as an argument is `NULL`.
> > >
> > > Addresses-Coverity: ("Dereference after null check")
> > > Fixes: d62c43a953ce ("dma-buf: Enable signaling on fence for 
> > > selftests")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
> > > ---
> > >   drivers/dma-buf/dma-fence.c | 9 ++++++++-
> > >   include/linux/dma-fence.h   | 2 +-
> > >   2 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > index 8aa8f8cb7071..4d2f13560d0f 100644
> > > --- a/drivers/dma-buf/dma-fence.c
> > > +++ b/drivers/dma-buf/dma-fence.c
> > > @@ -607,14 +607,21 @@ static bool 
> > > __dma_fence_enable_signaling(struct dma_fence *fence)
> > >    * This will request for sw signaling to be enabled, to make the 
> > > fence
> > >    * complete as soon as possible. This calls 
> > > &dma_fence_ops.enable_signaling
> > >    * internally.
> > > + *
> > > + * Returns 0 on success and a negative error value when @fence is 
> > > NULL.
> > >    */
> > > -void dma_fence_enable_sw_signaling(struct dma_fence *fence)
> > > +int dma_fence_enable_sw_signaling(struct dma_fence *fence)
> > >   {
> > >       unsigned long flags;
> > >   +    if (!fence)
> > > +        return -EINVAL;
> > > +
> > >       spin_lock_irqsave(fence->lock, flags);
> > >       __dma_fence_enable_signaling(fence);
> > >       spin_unlock_irqrestore(fence->lock, flags);
> > > +
> > > +    return 0;
> > >   }
> > >   EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
> > >   diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > > index ebe78bd3d121..1e4025e925e6 100644
> > > --- a/include/linux/dma-fence.h
> > > +++ b/include/linux/dma-fence.h
> > > @@ -399,7 +399,7 @@ int dma_fence_add_callback(struct dma_fence *fence,
> > >                  dma_fence_func_t func);
> > >   bool dma_fence_remove_callback(struct dma_fence *fence,
> > >                      struct dma_fence_cb *cb);
> > > -void dma_fence_enable_sw_signaling(struct dma_fence *fence);
> > > +int dma_fence_enable_sw_signaling(struct dma_fence *fence);
> > >     /**
> > >    * dma_fence_is_signaled_locked - Return an indication if the fence
> > > --
> > > 2.34.1