| Message ID | 20230810082333.972165-1-harperchen1110@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: vcodec: Fix potential array out-of-bounds in encoder queue_setup | expand |
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>
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,
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;
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
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; >
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;
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(+)