[v2] media: imx-jpeg: Fix some motion-jpeg decoding issues

[PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

Posted by ming.qian@oss.nxp.com 8 months, 3 weeks ago

From: Ming Qian <ming.qian@oss.nxp.com>

In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
buffer, but only return error if certain allocation failed.

Without cleanup the allocation failure, the next time it will return
success directly, but let some buffer be uninitialized.
It may result in accessing a null pointer.

Clean up if error occurs in the allocation.

Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
v2
- Add the Fixes tag

 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 0e6ee997284b..12661c177f5a 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
 	return -1;
 }
 
+static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
+{
+	/* free descriptor for decoding/encoding phase */
+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
+			  jpeg->slot_data.desc,
+			  jpeg->slot_data.desc_handle);
+	jpeg->slot_data.desc = NULL;
+	jpeg->slot_data.desc_handle = 0;
+
+	/* free descriptor for encoder configuration phase / decoder DHT */
+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
+			  jpeg->slot_data.cfg_desc,
+			  jpeg->slot_data.cfg_desc_handle);
+	jpeg->slot_data.cfg_desc_handle = 0;
+	jpeg->slot_data.cfg_desc = NULL;
+
+	/* free configuration stream */
+	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
+			  jpeg->slot_data.cfg_stream_vaddr,
+			  jpeg->slot_data.cfg_stream_handle);
+	jpeg->slot_data.cfg_stream_vaddr = NULL;
+	jpeg->slot_data.cfg_stream_handle = 0;
+
+	jpeg->slot_data.used = false;
+}
+
 static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
 {
 	struct mxc_jpeg_desc *desc;
@@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
 	return true;
 err:
 	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
+	mxc_jpeg_free_slot_data(jpeg);
 
 	return false;
 }
 
-static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
-{
-	/* free descriptor for decoding/encoding phase */
-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
-			  jpeg->slot_data.desc,
-			  jpeg->slot_data.desc_handle);
-
-	/* free descriptor for encoder configuration phase / decoder DHT */
-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
-			  jpeg->slot_data.cfg_desc,
-			  jpeg->slot_data.cfg_desc_handle);
-
-	/* free configuration stream */
-	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
-			  jpeg->slot_data.cfg_stream_vaddr,
-			  jpeg->slot_data.cfg_stream_handle);
-
-	jpeg->slot_data.used = false;
-}
-
 static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx *ctx,
 					       struct vb2_v4l2_buffer *src_buf,
 					       struct vb2_v4l2_buffer *dst_buf)
-- 
2.43.0-rc1

Re: [PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

Posted by Sebastian Fricke 8 months, 2 weeks ago

Hey Ming,

In the title I'd suggest:
media: imx-jpeg: Cleanup after an allocation error

To be a bit more concrete, enhance error handling can mean pretty much
anything.

On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>From: Ming Qian <ming.qian@oss.nxp.com>
>
>In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
>buffer, but only return error if certain allocation failed.
>
>Without cleanup the allocation failure, the next time it will return
>success directly, but let some buffer be uninitialized.
>It may result in accessing a null pointer.
>
>Clean up if error occurs in the allocation.

I'd suggest:

When allocation failures are not cleaned up by the driver, further allocation
errors will be false-positives, which will cause buffers to remain
uninitialized and cause NULL pointer dereferences.
Clean up the errors accordingly.

>
>Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
>Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>---
>v2
>- Add the Fixes tag
>
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
> 1 file changed, 27 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>index 0e6ee997284b..12661c177f5a 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>@@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
> 	return -1;
> }
>
>+static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>+{
>+	/* free descriptor for decoding/encoding phase */
>+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>+			  jpeg->slot_data.desc,
>+			  jpeg->slot_data.desc_handle);
>+	jpeg->slot_data.desc = NULL;
>+	jpeg->slot_data.desc_handle = 0;
>+
>+	/* free descriptor for encoder configuration phase / decoder DHT */
>+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>+			  jpeg->slot_data.cfg_desc,
>+			  jpeg->slot_data.cfg_desc_handle);
>+	jpeg->slot_data.cfg_desc_handle = 0;
>+	jpeg->slot_data.cfg_desc = NULL;
>+
>+	/* free configuration stream */
>+	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>+			  jpeg->slot_data.cfg_stream_vaddr,
>+			  jpeg->slot_data.cfg_stream_handle);
>+	jpeg->slot_data.cfg_stream_vaddr = NULL;
>+	jpeg->slot_data.cfg_stream_handle = 0;
>+
>+	jpeg->slot_data.used = false;
>+}
>+
> static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> {
> 	struct mxc_jpeg_desc *desc;
>@@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> 	return true;
> err:
> 	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
>+	mxc_jpeg_free_slot_data(jpeg);
>
> 	return false;
> }
>
>-static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>-{
>-	/* free descriptor for decoding/encoding phase */
>-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>-			  jpeg->slot_data.desc,
>-			  jpeg->slot_data.desc_handle);
>-
>-	/* free descriptor for encoder configuration phase / decoder DHT */
>-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>-			  jpeg->slot_data.cfg_desc,
>-			  jpeg->slot_data.cfg_desc_handle);
>-
>-	/* free configuration stream */
>-	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>-			  jpeg->slot_data.cfg_stream_vaddr,
>-			  jpeg->slot_data.cfg_stream_handle);
>-
>-	jpeg->slot_data.used = false;
>-}

Can you split the moving of the code from the changes you do?
Otherwise the reviewer is forced to get the diff manually.

Regards,
Sebastian Fricke

Re: [PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

Posted by Ming Qian(OSS) 8 months, 2 weeks ago

Hi Sebastian,


On 2025/4/5 19:39, Sebastian Fricke wrote:
> Hey Ming,
> 
> In the title I'd suggest:
> media: imx-jpeg: Cleanup after an allocation error
> 
> To be a bit more concrete, enhance error handling can mean pretty much
> anything.
> 

Thanks, I'll apply your suggestion

> On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
>> buffer, but only return error if certain allocation failed.
>>
>> Without cleanup the allocation failure, the next time it will return
>> success directly, but let some buffer be uninitialized.
>> It may result in accessing a null pointer.
>>
>> Clean up if error occurs in the allocation.
> 
> I'd suggest:
> 
> When allocation failures are not cleaned up by the driver, further 
> allocation
> errors will be false-positives, which will cause buffers to remain
> uninitialized and cause NULL pointer dereferences.
> Clean up the errors accordingly.
> 

Thanks, I'll apply your suggestion

>>
>> Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG 
>> Encoder/Decoder")
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>> v2
>> - Add the Fixes tag
>>
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
>> 1 file changed, 27 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 0e6ee997284b..12661c177f5a 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct 
>> mxc_jpeg_slot_data *slot_data)
>>     return -1;
>> }
>>
>> +static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>> +{
>> +    /* free descriptor for decoding/encoding phase */
>> +    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> +              jpeg->slot_data.desc,
>> +              jpeg->slot_data.desc_handle);
>> +    jpeg->slot_data.desc = NULL;
>> +    jpeg->slot_data.desc_handle = 0;
>> +
>> +    /* free descriptor for encoder configuration phase / decoder DHT */
>> +    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> +              jpeg->slot_data.cfg_desc,
>> +              jpeg->slot_data.cfg_desc_handle);
>> +    jpeg->slot_data.cfg_desc_handle = 0;
>> +    jpeg->slot_data.cfg_desc = NULL;
>> +
>> +    /* free configuration stream */
>> +    dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>> +              jpeg->slot_data.cfg_stream_vaddr,
>> +              jpeg->slot_data.cfg_stream_handle);
>> +    jpeg->slot_data.cfg_stream_vaddr = NULL;
>> +    jpeg->slot_data.cfg_stream_handle = 0;
>> +
>> +    jpeg->slot_data.used = false;
>> +}
>> +
>> static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>> {
>>     struct mxc_jpeg_desc *desc;
>> @@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct 
>> mxc_jpeg_dev *jpeg)
>>     return true;
>> err:
>>     dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", 
>> jpeg->slot_data.slot);
>> +    mxc_jpeg_free_slot_data(jpeg);
>>
>>     return false;
>> }
>>
>> -static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>> -{
>> -    /* free descriptor for decoding/encoding phase */
>> -    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> -              jpeg->slot_data.desc,
>> -              jpeg->slot_data.desc_handle);
>> -
>> -    /* free descriptor for encoder configuration phase / decoder DHT */
>> -    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> -              jpeg->slot_data.cfg_desc,
>> -              jpeg->slot_data.cfg_desc_handle);
>> -
>> -    /* free configuration stream */
>> -    dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>> -              jpeg->slot_data.cfg_stream_vaddr,
>> -              jpeg->slot_data.cfg_stream_handle);
>> -
>> -    jpeg->slot_data.used = false;
>> -}
> 
> Can you split the moving of the code from the changes you do?
> Otherwise the reviewer is forced to get the diff manually.

Sure, this will be done in v3

> 
> Regards,
> Sebastian Fricke

Re: [PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

Posted by Frank Li 8 months, 3 weeks ago

On Fri, Mar 28, 2025 at 02:30:50PM +0800, ming.qian@oss.nxp.com wrote:
> From: Ming Qian <ming.qian@oss.nxp.com>
>
> In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
> buffer, but only return error if certain allocation failed.
>
> Without cleanup the allocation failure, the next time it will return
> success directly, but let some buffer be uninitialized.
> It may result in accessing a null pointer.
>
> Clean up if error occurs in the allocation.
>
> Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
> v2
> - Add the Fixes tag
>
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
>  1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 0e6ee997284b..12661c177f5a 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
>  	return -1;
>  }
>
> +static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> +{
> +	/* free descriptor for decoding/encoding phase */
> +	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> +			  jpeg->slot_data.desc,
> +			  jpeg->slot_data.desc_handle);
> +	jpeg->slot_data.desc = NULL;
> +	jpeg->slot_data.desc_handle = 0;
> +
> +	/* free descriptor for encoder configuration phase / decoder DHT */
> +	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> +			  jpeg->slot_data.cfg_desc,
> +			  jpeg->slot_data.cfg_desc_handle);
> +	jpeg->slot_data.cfg_desc_handle = 0;
> +	jpeg->slot_data.cfg_desc = NULL;
> +
> +	/* free configuration stream */
> +	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> +			  jpeg->slot_data.cfg_stream_vaddr,
> +			  jpeg->slot_data.cfg_stream_handle);
> +	jpeg->slot_data.cfg_stream_vaddr = NULL;
> +	jpeg->slot_data.cfg_stream_handle = 0;
> +
> +	jpeg->slot_data.used = false;
> +}
> +
>  static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>  {
>  	struct mxc_jpeg_desc *desc;
> @@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>  	return true;
>  err:
>  	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
> +	mxc_jpeg_free_slot_data(jpeg);
>
>  	return false;
>  }
>
> -static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> -{
> -	/* free descriptor for decoding/encoding phase */
> -	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> -			  jpeg->slot_data.desc,
> -			  jpeg->slot_data.desc_handle);
> -
> -	/* free descriptor for encoder configuration phase / decoder DHT */
> -	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> -			  jpeg->slot_data.cfg_desc,
> -			  jpeg->slot_data.cfg_desc_handle);
> -
> -	/* free configuration stream */
> -	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> -			  jpeg->slot_data.cfg_stream_vaddr,
> -			  jpeg->slot_data.cfg_stream_handle);
> -
> -	jpeg->slot_data.used = false;
> -}
> -
>  static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx *ctx,
>  					       struct vb2_v4l2_buffer *src_buf,
>  					       struct vb2_v4l2_buffer *dst_buf)
> --
> 2.43.0-rc1
>

[PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation
[PATCH v2 2/3] media: imx-jpeg: Change the pattern size to 128x64
[PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg