Fwd: [PATCH 1/1] v4l: Avoid unaligned access warnings when printing 4cc modifiers

[Nick Desaulniers <ndesaulniers@google.com>]

---------- Forwarded message ---------
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Mon, Jan 10, 2022 at 3:11 PM
Subject: Re: [PATCH 1/1] v4l: Avoid unaligned access warnings when
printing 4cc modifiers
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: <linux-media@vger.kernel.org>, Andy Shevchenko
<andriy.shevchenko@linux.intel.com>, <hverkuil@xs4all.nl>


On Mon, Jan 10, 2022 at 2:48 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Pointers V4L2 pixelformat and dataformat fields in a few packed structs
> are directly passed to printk family of functions.

I would rephrase the below statement...

> This could result in an
> unaligned access albeit no such possibility appears to exist at the
> moment i.e. this clang warning appears to be a false positive.

...to:

warning: taking address of packed member 'pixelformat' of class or
structure 'v4l2_pix_format_mplane' may result in an unaligned pointer
value [-Waddress-of-packed-member]

The warning is correct; because `struct v4l2_pix_format_mplane` is
__packed, it's members also have __aligned(1).  Taking the address of
such members results in the use of underaligned pointers which is UB
and may be caught by UBSAN or fault on architectures without unaligned
loads should the struct instance happen to be allocated without any
natural alignment.

>
> Address the warning by copying the pixelformat or dataformat value to a
> local variable first.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Andy, Nick,
>
> How about this one?
>
> I believe it does address the clang warning although I haven't tested it.

LGTM. Thanks Sakari and Andy for pursuing this. Just a minor nit on my
side about the framing of this warning being a false positive; I don't
think it is.  With that amended,

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Kind regards,
> Sakari
>
>  drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 9ac557b8e146..642cb90f457c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -279,8 +279,8 @@ static void v4l_print_format(const void *arg, bool write_only)
>         const struct v4l2_vbi_format *vbi;
>         const struct v4l2_sliced_vbi_format *sliced;
>         const struct v4l2_window *win;
> -       const struct v4l2_sdr_format *sdr;
>         const struct v4l2_meta_format *meta;
> +       u32 pixelformat;
>         u32 planes;
>         unsigned i;
>
> @@ -299,8 +299,9 @@ static void v4l_print_format(const void *arg, bool write_only)
>         case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>         case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>                 mp = &p->fmt.pix_mp;
> +               pixelformat = mp->pixelformat;
>                 pr_cont(", width=%u, height=%u, format=%p4cc, field=%s, colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n",
> -                       mp->width, mp->height, &mp->pixelformat,
> +                       mp->width, mp->height, &pixelformat,
>                         prt_names(mp->field, v4l2_field_names),
>                         mp->colorspace, mp->num_planes, mp->flags,
>                         mp->ycbcr_enc, mp->quantization, mp->xfer_func);
> @@ -343,14 +344,15 @@ static void v4l_print_format(const void *arg, bool write_only)
>                 break;
>         case V4L2_BUF_TYPE_SDR_CAPTURE:
>         case V4L2_BUF_TYPE_SDR_OUTPUT:
> -               sdr = &p->fmt.sdr;
> -               pr_cont(", pixelformat=%p4cc\n", &sdr->pixelformat);
> +               pixelformat = p->fmt.sdr.pixelformat;
> +               pr_cont(", pixelformat=%p4cc\n", &pixelformat);
>                 break;
>         case V4L2_BUF_TYPE_META_CAPTURE:
>         case V4L2_BUF_TYPE_META_OUTPUT:
>                 meta = &p->fmt.meta;
> +               pixelformat = meta->dataformat;
>                 pr_cont(", dataformat=%p4cc, buffersize=%u\n",
> -                       &meta->dataformat, meta->buffersize);
> +                       &pixelformat, meta->buffersize);
>                 break;
>         }
>  }
> --
> 2.30.2
>


--
Thanks,
~Nick Desaulniers


-- 
Thanks,
~Nick Desaulniers