linux-kernel - Re: [PATCH 2/3] media: dvb: Fix dtvs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCvkRWZXuG7dfw0WXvgT+LHQqG3fx9F1M2P0_9dkB9VOKA@mail.gmail.com>
Date: Fri, 12 Apr 2024 17:00:05 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans Verkuil <hverkuil-cisco@...all.nl>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

Hi Hans

On Fri, 12 Apr 2024 at 16:21, Hans Verkuil <hverkuil-cisco@...all.nl> wrote:
>
> On 10/04/2024 14:24, Ricardo Ribalda wrote:
> > The structure is packed, which requires that all its fields need to be
> > also packed.
> >
> > ./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> >
> > Explicitly set the inner union as packed.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> >  include/uapi/linux/dvb/frontend.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
> > index 7e0983b987c2d..8d38c6befda8d 100644
> > --- a/include/uapi/linux/dvb/frontend.h
> > +++ b/include/uapi/linux/dvb/frontend.h
> > @@ -854,7 +854,7 @@ struct dtv_stats {
> >       union {
> >               __u64 uvalue;   /* for counters and relative scales */
> >               __s64 svalue;   /* for 0.001 dB measures */
> > -     };
> > +     }  __attribute__ ((packed));
> >  } __attribute__ ((packed));
>
> This is used in the public API, and I think this change can cause ABI changes.
>
> Can you compare the layouts? Also between gcc and llvm since gcc never warned
> about this.

The pahole output looks the same in both cases:

https://godbolt.org/z/oK4desv7Y
vs
https://godbolt.org/z/E36MjPr7v

And it is also the same for all the compiler versions that I tried.


struct dtv_stats {
uint8_t                    scale;                /*     0     1 */
union {
uint64_t           uvalue;               /*     1     8 */
int64_t            svalue;               /*     1     8 */
};                                               /*     1     8 */

/* size: 9, cachelines: 1, members: 2 */
/* last cacheline: 9 bytes */
} __attribute__((__packed__));



struct dtv_stats {
uint8_t scale; /* 0 1 */
union {
uint64_t uvalue; /* 1 8 */
int64_t svalue; /* 1 8 */
}; /* 1 8 */

/* size: 9, cachelines: 1, members: 2 */
/* last cacheline: 9 bytes */
} __attribute__((__packed__));


>
> I'm not going to accept this unless it is clear that there are no ABI changes.

Is there something else that I can try?

Regards!

>
> Note that the ABI test in the build scripts only tests V4L2 at the moment,
> not the DVB API.
>
> Regards,
>
>         Hans
>


-- 
Ricardo Ribalda