media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

[PATCH v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

Posted by Wei Chen 1 year, 3 months ago

variable *nplanes is provided by user via system call argument. The
possible value of q_data->fmt->num_planes is 1-3, while the value
of *nplanes can be 1-8. The array access by index i can cause array
out-of-bounds.

Fix this bug by checking *nplanes against the array size.

Fixes: 4e855a6efa54 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
Signed-off-by: Wei Chen <harperchen1110@gmail.com>
Cc: stable@vger.kernel.org
---
Changes in v2:
- Add Fixes tag and CC stable email address
- Change the title to be more expressive

 drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
index 9ff439a50f53..9e8817863cb8 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
@@ -821,6 +821,8 @@ static int vb2ops_venc_queue_setup(struct vb2_queue *vq,
 		return -EINVAL;
 
 	if (*nplanes) {
+		if (*nplanes != q_data->fmt->num_planes)
+			return -EINVAL;
 		for (i = 0; i < *nplanes; i++)
 			if (sizes[i] < q_data->sizeimage[i])
 				return -EINVAL;
-- 
2.25.1

Re: [PATCH v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

Posted by Nicolas Dufresne 1 year, 3 months ago

Hi,

Le jeudi 10 août 2023 à 08:23 +0000, Wei Chen a écrit :
> variable *nplanes is provided by user via system call argument. The
> possible value of q_data->fmt->num_planes is 1-3, while the value
> of *nplanes can be 1-8. The array access by index i can cause array
> out-of-bounds.
> 
> Fix this bug by checking *nplanes against the array size.
> 
> Fixes: 4e855a6efa54 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
> Signed-off-by: Wei Chen <harperchen1110@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> Changes in v2:
> - Add Fixes tag and CC stable email address
> - Change the title to be more expressive
> 
>  drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
> index 9ff439a50f53..9e8817863cb8 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
> @@ -821,6 +821,8 @@ static int vb2ops_venc_queue_setup(struct vb2_queue *vq,
>  		return -EINVAL;
>  
>  	if (*nplanes) {
> +		if (*nplanes != q_data->fmt->num_planes)
> +			return -EINVAL;

I don't think the claim really exists. 	For this driver, when *nplane is set,
it will be:


        case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
                requested_planes = f->fmt.pix_mp.num_planes;
                if (requested_planes == 0 ||
                    requested_planes > VIDEO_MAX_PLANES)
                        return -EINVAL;
                for (i = 0; i < requested_planes; i++)
                        requested_sizes[i] =
                                f->fmt.pix_mp.plane_fmt[i].sizeimage;
                break;

Or the value the driver have set it in the previous call with *nplane == 0. So
unless there is a bug, this should not happen, and more importantly, the core
should not let that happen, meaning it should not be driver jobs to validate
this.

my 2 cents,
Nicolas


>  		for (i = 0; i < *nplanes; i++)
>  			if (sizes[i] < q_data->sizeimage[i])
>  				return -EINVAL;

Re: [PATCH v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

Posted by Hans Verkuil 1 year, 2 months ago

On 10/08/2023 15:58, Nicolas Dufresne wrote:
> Hi,
> 
> Le jeudi 10 août 2023 à 08:23 +0000, Wei Chen a écrit :
>> variable *nplanes is provided by user via system call argument. The
>> possible value of q_data->fmt->num_planes is 1-3, while the value
>> of *nplanes can be 1-8. The array access by index i can cause array
>> out-of-bounds.
>>
>> Fix this bug by checking *nplanes against the array size.
>>
>> Fixes: 4e855a6efa54 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
>> Signed-off-by: Wei Chen <harperchen1110@gmail.com>
>> Cc: stable@vger.kernel.org
>> ---
>> Changes in v2:
>> - Add Fixes tag and CC stable email address
>> - Change the title to be more expressive
>>
>>  drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
>> index 9ff439a50f53..9e8817863cb8 100644
>> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
>> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
>> @@ -821,6 +821,8 @@ static int vb2ops_venc_queue_setup(struct vb2_queue *vq,
>>  		return -EINVAL;
>>  
>>  	if (*nplanes) {
>> +		if (*nplanes != q_data->fmt->num_planes)
>> +			return -EINVAL;
> 
> I don't think the claim really exists. 	For this driver, when *nplane is set,
> it will be:
> 
> 
>         case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>                 requested_planes = f->fmt.pix_mp.num_planes;
>                 if (requested_planes == 0 ||
>                     requested_planes > VIDEO_MAX_PLANES)
>                         return -EINVAL;
>                 for (i = 0; i < requested_planes; i++)
>                         requested_sizes[i] =
>                                 f->fmt.pix_mp.plane_fmt[i].sizeimage;
>                 break;
> 
> Or the value the driver have set it in the previous call with *nplane == 0. So
> unless there is a bug, this should not happen, and more importantly, the core
> should not let that happen, meaning it should not be driver jobs to validate
> this.
> 
> my 2 cents,
> Nicolas
> 
> 
>>  		for (i = 0; i < *nplanes; i++)
>>  			if (sizes[i] < q_data->sizeimage[i])

It's q_data->sizeimage that has only 3 elements, so if *nplanes is 4, then
this will fail with an OOB.

However, I think the check should really happen in the vb2 core. If no
buffers have been allocated yet, then the queue_setup callback will set
the number of planes based on the current format.

If CREATE_BUFS is called afterwards, then *nplanes is set to the number
of planes that is specified in the format field of struct v4l2_create_buffers.

The core clips that value to the range [1..VIDEO_MAX_PLANES], but otherwise it
places no restrictions.

I was afraid that this would cause serious problems if fewer planes are
requested than the current format needs, but in that case the size checks
in the buf_prepare callback of the driver will fail (since one or more planes
will have size 0).

The idea behind allowing userspace to allocate different number of planes
in CREATE_BUFS really makes little sense and there are no drivers that rely
on this. It would be much better if the vb2 core would return -EINVAL if
the requested number of planes does not match that of what is used for already
allocated buffers.

If we ever get drivers that for some reason want more flexibility, then those
drivers can set a special flag indicating that they want to check this themselves.

I'll post a patch adding the check to the vb2 core.

Regards,

	Hans

>>  				return -EINVAL;
>

Re: [PATCH v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

Posted by Eugen Hristev 1 year, 3 months ago

Hi Wei,

On 8/10/23 11:23, Wei Chen wrote:
> variable *nplanes is provided by user via system call argument. The
> possible value of q_data->fmt->num_planes is 1-3, while the value
> of *nplanes can be 1-8. The array access by index i can cause array
> out-of-bounds.
> 
> Fix this bug by checking *nplanes against the array size.
> 
> Fixes: 4e855a6efa54 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
> Signed-off-by: Wei Chen <harperchen1110@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> Changes in v2:
> - Add Fixes tag and CC stable email address
I guess stable needs to be added by the maintainer, not by the submitter

> - Change the title to be more expressive

Subject message should include mediatek I believe, as `media: vcodec:` 
does not reference the mediatek vcodec.

> 
>   drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
> index 9ff439a50f53..9e8817863cb8 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc.c
> @@ -821,6 +821,8 @@ static int vb2ops_venc_queue_setup(struct vb2_queue *vq,
>   		return -EINVAL;
>   
>   	if (*nplanes) {
> +		if (*nplanes != q_data->fmt->num_planes)
> +			return -EINVAL;

Have you run v4l2-compliance on the device to make sure nothing is broken ?

>   		for (i = 0; i < *nplanes; i++)
>   			if (sizes[i] < q_data->sizeimage[i])
>   				return -EINVAL;


Greetings,

Re: [PATCH v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

Posted by Greg KH 1 year, 3 months ago

On Thu, Aug 10, 2023 at 12:12:39PM +0300, Eugen Hristev wrote:
> Hi Wei,
> 
> On 8/10/23 11:23, Wei Chen wrote:
> > variable *nplanes is provided by user via system call argument. The
> > possible value of q_data->fmt->num_planes is 1-3, while the value
> > of *nplanes can be 1-8. The array access by index i can cause array
> > out-of-bounds.
> > 
> > Fix this bug by checking *nplanes against the array size.
> > 
> > Fixes: 4e855a6efa54 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
> > Signed-off-by: Wei Chen <harperchen1110@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> > Changes in v2:
> > - Add Fixes tag and CC stable email address
> I guess stable needs to be added by the maintainer, not by the submitter

It's easiest if it is added by the submitter, makes the maintainer's job
much easier.

thanks,

greg k-h

Re: [PATCH v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup

Posted by Chen-Yu Tsai 1 year, 3 months ago

On Thu, Aug 10, 2023 at 4:23 PM Wei Chen <harperchen1110@gmail.com> wrote:
>
> variable *nplanes is provided by user via system call argument. The
> possible value of q_data->fmt->num_planes is 1-3, while the value
> of *nplanes can be 1-8. The array access by index i can cause array
> out-of-bounds.
>
> Fix this bug by checking *nplanes against the array size.
>
> Fixes: 4e855a6efa54 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
> Signed-off-by: Wei Chen <harperchen1110@gmail.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>