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

[Hans Verkuil]

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@xxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>> 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;
>