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@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. 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 > > >
