diff mbox series

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

Message ID 20250327023710.549-2-ming.qian@oss.nxp.com (mailing list archive)
State New
Headers show
Series media: imx-jpeg: Fix some motion-jpeg decoding issues | expand

Commit Message

Ming Qian(OSS) March 27, 2025, 2:37 a.m. UTC 
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.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
 1 file changed, 27 insertions(+), 20 deletions(-)

Comments

Frank Li March 27, 2025, 2:35 p.m. UTC | #1 
On Thu, Mar 27, 2025 at 10:37:05AM +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.
>
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>

Look like it is bug fix, add fixes tags

Frank
> ---
>  .../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
>
Ming Qian(OSS) March 28, 2025, 1:22 a.m. UTC | #2 
Hi Frank,

On 2025/3/27 22:35, Frank Li wrote:
> On Thu, Mar 27, 2025 at 10:37:05AM +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.
>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> 
> Look like it is bug fix, add fixes tags

Yes, I missed it, I will add it in v2

Thanks,
Ming

> 
> Frank
>> ---
>>   .../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
>>
diff mbox series

Patch

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)