| Message ID | 20240715102425.1244918-1-naush@raspberrypi.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: pispbe: Protect against left-shift wrap in V4L2_COLORSPACE_MASK() | expand |
Hi Naush On Mon, Jul 15, 2024 at 11:24:25AM GMT, Naushir Patuck wrote: > Ensure that the user requested colorspace value does not wrap when > using the V4L2_COLORSPACE_MASK() macro. If the requested colorspace > value >= BIT_PER_LONG, revert to the default colorspace for the given > format. > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Thanks for handling this > --- > drivers/media/platform/raspberrypi/pisp_be/pisp_be.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > index e74df5b116dc..bd5d77c691d3 100644 > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > @@ -1124,8 +1124,9 @@ static void pispbe_try_format(struct v4l2_format *f, struct pispbe_node *node) > * not supported. This also catches the case when the "default" > * colour space was requested (as that's never in the mask). > */ > - if (!(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & > - fmt->colorspace_mask)) > + if (f->fmt.pix_mp.colorspace >= BITS_PER_LONG || > + !(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & > + fmt->colorspace_mask)) > f->fmt.pix_mp.colorspace = fmt->colorspace_default; Isn't it better handled in the macro definition itself so that future usages of the V4L2_COLORSPACE_MASK() macro won't need to be protected like this one ? Would this silence the smatch warning ? -#define V4L2_COLORSPACE_MASK(colorspace) BIT(colorspace) +#define V4L2_COLORSPACE_MASK(c) BIT((c) < V4L2_COLORSPACE_LAST ? \ + (c) : V4L2_COLORSPACE_LAST) > > /* In all cases, we only support the defaults for these: */ > -- > 2.34.1 >
Hi Jacopo, On Mon, 15 Jul 2024 at 15:30, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Mon, Jul 15, 2024 at 11:24:25AM GMT, Naushir Patuck wrote: > > Ensure that the user requested colorspace value does not wrap when > > using the V4L2_COLORSPACE_MASK() macro. If the requested colorspace > > value >= BIT_PER_LONG, revert to the default colorspace for the given > > format. > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Thanks for handling this > > > --- > > drivers/media/platform/raspberrypi/pisp_be/pisp_be.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > index e74df5b116dc..bd5d77c691d3 100644 > > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > @@ -1124,8 +1124,9 @@ static void pispbe_try_format(struct v4l2_format *f, struct pispbe_node *node) > > * not supported. This also catches the case when the "default" > > * colour space was requested (as that's never in the mask). > > */ > > - if (!(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & > > - fmt->colorspace_mask)) > > + if (f->fmt.pix_mp.colorspace >= BITS_PER_LONG || > > + !(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & > > + fmt->colorspace_mask)) > > f->fmt.pix_mp.colorspace = fmt->colorspace_default; > > Isn't it better handled in the macro definition itself so that future > usages of the V4L2_COLORSPACE_MASK() macro won't need to be protected > like this one ? > > Would this silence the smatch warning ? > > -#define V4L2_COLORSPACE_MASK(colorspace) BIT(colorspace) > +#define V4L2_COLORSPACE_MASK(c) BIT((c) < V4L2_COLORSPACE_LAST ? \ > + (c) : V4L2_COLORSPACE_LAST) > I did consider this, but did not like the undesired behavior it would introduce. With your suggested change above, we now switch the color space to an unsupported value (V4L2_COLORSPACE_DCI_P3, assuming we shift by V4L2_COLORSPACE_LAST - 1) if the user passed an invalid value. Of course, this does subsequently get fixed when used in pispbe_try_format(), but not for the usage in pisp_be_formats.h. In my original change, we revert to the default for the requested format instead. Although, perhaps my test should be if (!v4l2_is_colorspace_valid(f->fmt.pix_mp.colorspace) ... instead. I'm ok with changing the macro considering the UAPI is unlikely to change, and usage in pisp_be_formats.h will likely not hit the problem any time. Thoughts? Naush > > > > > > /* In all cases, we only support the defaults for these: */ > > -- > > 2.34.1 > >
Hi Naush On Tue, Jul 16, 2024 at 08:57:56AM GMT, Naushir Patuck wrote: > Hi Jacopo, > > On Mon, 15 Jul 2024 at 15:30, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Mon, Jul 15, 2024 at 11:24:25AM GMT, Naushir Patuck wrote: > > > Ensure that the user requested colorspace value does not wrap when > > > using the V4L2_COLORSPACE_MASK() macro. If the requested colorspace > > > value >= BIT_PER_LONG, revert to the default colorspace for the given > > > format. > > > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Thanks for handling this > > > > > --- > > > drivers/media/platform/raspberrypi/pisp_be/pisp_be.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > index e74df5b116dc..bd5d77c691d3 100644 > > > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > @@ -1124,8 +1124,9 @@ static void pispbe_try_format(struct v4l2_format *f, struct pispbe_node *node) > > > * not supported. This also catches the case when the "default" > > > * colour space was requested (as that's never in the mask). > > > */ > > > - if (!(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & > > > - fmt->colorspace_mask)) > > > + if (f->fmt.pix_mp.colorspace >= BITS_PER_LONG || > > > + !(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & > > > + fmt->colorspace_mask)) > > > f->fmt.pix_mp.colorspace = fmt->colorspace_default; > > > > Isn't it better handled in the macro definition itself so that future > > usages of the V4L2_COLORSPACE_MASK() macro won't need to be protected > > like this one ? > > > > Would this silence the smatch warning ? > > > > -#define V4L2_COLORSPACE_MASK(colorspace) BIT(colorspace) > > +#define V4L2_COLORSPACE_MASK(c) BIT((c) < V4L2_COLORSPACE_LAST ? \ > > + (c) : V4L2_COLORSPACE_LAST) > > > > I did consider this, but did not like the undesired behavior it would > introduce. With your suggested change above, we now switch the color > space to an unsupported value (V4L2_COLORSPACE_DCI_P3, assuming we > shift by V4L2_COLORSPACE_LAST - 1) if the user passed an invalid > value. Of course, this does subsequently get fixed when used in > pispbe_try_format(), but not for the usage in pisp_be_formats.h. In > my original change, we revert to the default for the requested format > instead. Although, perhaps my test should be if > (!v4l2_is_colorspace_valid(f->fmt.pix_mp.colorspace) ... instead. Keep in mind the core 'sanitizes' the colorspace for you (see the usage of v4l_sanitize_colorspace() in drivers/media/v4l2-core/v4l2-ioctl.c) so a fix is need only to silence smatch not to actually address any run-time issue. > > I'm ok with changing the macro considering the UAPI is unlikely to > change, and usage in pisp_be_formats.h will likely not hit the problem > any time. Thoughts? We could re-use v4l_sanitize_colorspace() but again, this change only serve to please smatch. Thanks j > > Naush > > > > > > > > > > > /* In all cases, we only support the defaults for these: */ > > > -- > > > 2.34.1 > > >
On Tue, Jul 16, 2024 at 10:48:20AM +0200, Jacopo Mondi wrote: > > I did consider this, but did not like the undesired behavior it would > > introduce. With your suggested change above, we now switch the color > > space to an unsupported value (V4L2_COLORSPACE_DCI_P3, assuming we > > shift by V4L2_COLORSPACE_LAST - 1) if the user passed an invalid > > value. Of course, this does subsequently get fixed when used in > > pispbe_try_format(), but not for the usage in pisp_be_formats.h. In > > my original change, we revert to the default for the requested format > > instead. Although, perhaps my test should be if > > (!v4l2_is_colorspace_valid(f->fmt.pix_mp.colorspace) ... instead. > > Keep in mind the core 'sanitizes' the colorspace for you (see the > usage of v4l_sanitize_colorspace() in > drivers/media/v4l2-core/v4l2-ioctl.c) so a fix is need only to silence > smatch not to actually address any run-time issue. > Ah. That's true. Let's just ignore this warning in that case... Smatch would have an easier time parsing these if we passed p instead of arg to return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg) in v4l_try_fmt(). struct v4l2_format *p = arg; v4l_sanitize_format(p); return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg); I can just filter out that call to say that when v4l_try_fmt() calls ops->vidioc_try_fmt_vid_cap_mplane() "arg" variable is trusted data. regards, dan carpenter
On Tue, 16 Jul 2024 at 14:30, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Tue, Jul 16, 2024 at 10:48:20AM +0200, Jacopo Mondi wrote: > > > I did consider this, but did not like the undesired behavior it would > > > introduce. With your suggested change above, we now switch the color > > > space to an unsupported value (V4L2_COLORSPACE_DCI_P3, assuming we > > > shift by V4L2_COLORSPACE_LAST - 1) if the user passed an invalid > > > value. Of course, this does subsequently get fixed when used in > > > pispbe_try_format(), but not for the usage in pisp_be_formats.h. In > > > my original change, we revert to the default for the requested format > > > instead. Although, perhaps my test should be if > > > (!v4l2_is_colorspace_valid(f->fmt.pix_mp.colorspace) ... instead. > > > > Keep in mind the core 'sanitizes' the colorspace for you (see the > > usage of v4l_sanitize_colorspace() in > > drivers/media/v4l2-core/v4l2-ioctl.c) so a fix is need only to silence > > smatch not to actually address any run-time issue. > > > > Ah. That's true. Let's just ignore this warning in that case... > > Smatch would have an easier time parsing these if we passed p instead of > arg to return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg) in > v4l_try_fmt(). > > struct v4l2_format *p = arg; > > v4l_sanitize_format(p); > > return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg); > > I can just filter out that call to say that when v4l_try_fmt() calls > ops->vidioc_try_fmt_vid_cap_mplane() "arg" variable is trusted data. Sounds good, I'll drop this patch then. Naush
diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c index e74df5b116dc..bd5d77c691d3 100644 --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c @@ -1124,8 +1124,9 @@ static void pispbe_try_format(struct v4l2_format *f, struct pispbe_node *node) * not supported. This also catches the case when the "default" * colour space was requested (as that's never in the mask). */ - if (!(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & - fmt->colorspace_mask)) + if (f->fmt.pix_mp.colorspace >= BITS_PER_LONG || + !(V4L2_COLORSPACE_MASK(f->fmt.pix_mp.colorspace) & + fmt->colorspace_mask)) f->fmt.pix_mp.colorspace = fmt->colorspace_default; /* In all cases, we only support the defaults for these: */
Ensure that the user requested colorspace value does not wrap when using the V4L2_COLORSPACE_MASK() macro. If the requested colorspace value >= BIT_PER_LONG, revert to the default colorspace for the given format. Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- drivers/media/platform/raspberrypi/pisp_be/pisp_be.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)