diff mbox series

media: pispbe: Protect against left-shift wrap in V4L2_COLORSPACE_MASK()

Message ID 20240715102425.1244918-1-naush@raspberrypi.com (mailing list archive)
State Rejected
Headers
Series media: pispbe: Protect against left-shift wrap in V4L2_COLORSPACE_MASK() |

Commit Message

Naushir Patuck July 15, 2024, 10:24 a.m. UTC 
  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(-)

Comments

Jacopo Mondi July 15, 2024, 2:30 p.m. UTC | #1 
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
>
Naushir Patuck July 16, 2024, 7:57 a.m. UTC | #2 
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
> >
Jacopo Mondi July 16, 2024, 8:48 a.m. UTC | #3 
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
> > >
Dan Carpenter July 16, 2024, 1:29 p.m. UTC | #4 
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
Naushir Patuck July 18, 2024, 7:03 a.m. UTC | #5 
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 mbox series

Patch

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: */