linux-kernel - Re: [PATCH v2 6/8] media: platform: amd: isp4 video node and buffers handling added

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b5cd1017-56c8-4d09-bc1d-203774827827@amd.com>
Date: Tue, 29 Jul 2025 14:08:05 +0800
From: "Du, Bin" <bin.du@....com>
To: Sultan Alsawaf <sultan@...neltoast.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
 laurent.pinchart+renesas@...asonboard.com, bryan.odonoghue@...aro.org,
 sakari.ailus@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
 gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
 bin.du@....com
Subject: Re: [PATCH v2 6/8] media: platform: amd: isp4 video node and buffers
 handling added

Many thanks Sultan, will try your modifications in this and other mails 
together and let you know the result

On 7/27/2025 5:41 AM, Sultan Alsawaf wrote:
> On Fri, Jul 25, 2025 at 05:22:41PM +0800, Du, Bin wrote:
>>>> +static struct dma_buf *isp4vid_vb2_get_dmabuf(struct vb2_buffer *vb,
>>>> +					      void *buf_priv,
>>>> +					      unsigned long flags)
>>>> +{
>>>> +	struct isp4vid_vb2_buf *buf = buf_priv;
>>>> +	struct dma_buf *dbuf;
>>>> +
>>>> +	if (buf->dbuf) {
>>>> +		dev_dbg(buf->dev,
>>>> +			"dbuf already created, reuse implicit dbuf\n");
>>>> +		dbuf = buf->dbuf;
>>>
>>> The dmabuf is reused here without taking a reference to it. When the get_dmabuf
>>> memop is called by vb2_core_expbuf(), it assumes that a reference to the dmabuf
>>> is acquired. So you need to add `get_dma_buf(dbuf)` here.
>> After test, we found we can't add get_dma_buf(dbuf) here because it will
>> make cheese APP fail to open camera with following error:
>> amdgpu: [drm] *ERROR* failed to alloc gart kernel buffer (-28)
> 
> I see, it's because buf->is_expbuf is set to true even for the implicit dbuf, so
> the initial reference on the implicit dbuf is never put, causing a leak.
> 
> Also, the refcount increment in isp4vid_vb2_get_dmabuf() is done every time even
> when reusing the existing dbuf, but releasing the dbuf will only do a single
> refcount decrement. This also causes a leak.
> 
> And, isp4vid_get_dmabuf() may fail but isp4vid_vb2_get_dmabuf() doesn't check
> the return value, so there may be another leak when isp4vid_get_dmabuf() fails
> because of the refcount increment. The refcount increment and setting of
> buf->is_expbuf to true should only be done on success.
> 
> I have fixed all of these isp4vid_vb2_get_dmabuf() issues in the following diff,
> please try it:
> 
> --- a/drivers/media/platform/amd/isp4/isp4_video.c
> +++ b/drivers/media/platform/amd/isp4/isp4_video.c
> @@ -476,18 +476,22 @@ static struct dma_buf *isp4vid_vb2_get_dmabuf(struct vb2_buffer *vb,
>   					      unsigned long flags)
>   {
>   	struct isp4vid_vb2_buf *buf = buf_priv;
> -	struct dma_buf *dbuf;
> +	struct dma_buf *dbuf = buf->dbuf;
>   
> -	if (buf->dbuf) {
> +	if (dbuf) {
>   		dev_dbg(buf->dev,
> -			"dbuf already created, reuse implicit dbuf\n");
> -		dbuf = buf->dbuf;
> +			"dbuf already created, reuse %s dbuf\n",
> +			buf->is_expbuf ? "exported" : "implicit");
> +		get_dma_buf(dbuf);
>   	} else {
>   		dbuf = isp4vid_get_dmabuf(vb, buf_priv, flags);
> +		if (!dbuf)
> +			return NULL;
> +
>   		dev_dbg(buf->dev, "created new dbuf\n");
> +		buf->is_expbuf = true;
> +		refcount_inc(&buf->refcount);
>   	}
> -	buf->is_expbuf = true;
> -	refcount_inc(&buf->refcount);
>   
>   	dev_dbg(buf->dev, "buf exported, refcount %d\n",
>   		buf->refcount.refs.counter);
> --
> 
>>>> +		dev_warn(buf->dev, "ignore buffer free, refcount %u > 0",
>>>> +			 refcount_read(&buf->refcount));
>>>
>>> This refcount_read() is a possible use-after-free because `buf` is accessed
>>> after isp4vid_vb2_put() puts its reference to `buf`. So something else could put
>>> the last reference to `buf` and free it after this refcount dec but before the
>>> refcount_read(). Maybe just remove this dev_warn() entirely?
>>>
>> The warning is important to debug mem related issue, plan to keep it but
>> without accessing buf or buf->refcount here. Do you think it acceptible?
> 
> Yes, that sounds good. So something like this:
> `dev_warn(buf->dev, "ignore buffer free, refcount > 0");`
> 
> Sultan