Hi Jacopo, On Mon, 15 Jul 2024 at 15:30, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> > > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx> > > 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 > >
