linux-kernel - Re: [PATCH 05/16] bus: mhi: core: Add support for ringing channel/event ring doorbells

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]

Date:   Sat, 25 Jan 2020 14:46:31 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Jeffrey Hugo <jhugo@...eaurora.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        arnd@...db.de, smohanad@...eaurora.org, kvalo@...eaurora.org,
        bjorn.andersson@...aro.org, hemantk@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/16] bus: mhi: core: Add support for ringing
 channel/event ring doorbells

On Fri, Jan 24, 2020 at 03:51:12PM -0700, Jeffrey Hugo wrote:
> > +struct mhi_event_ctxt {
> > +	u32 reserved : 8;
> > +	u32 intmodc : 8;
> > +	u32 intmodt : 16;
> > +	u32 ertype;
> > +	u32 msivec;
> > +
> > +	u64 rbase __packed __aligned(4);
> > +	u64 rlen __packed __aligned(4);
> > +	u64 rp __packed __aligned(4);
> > +	u64 wp __packed __aligned(4);
> > +};
> 
> This is the struct that is shared with the device, correct?  Surely it needs
> to be packed then?  Seems like you'd expect some padding between msivec and
> rbase on a 64-bit system otherwise, which is probably not intended.
> 
> Also I strongly dislike bitfields in structures which are shared with
> another system since the C specification doesn't define how they are
> implemented, therefore you can run into issues where different compilers
> decide to implement the actual backing memory differently.  I know its less
> convinent, but I would prefer the use of bitmasks for these fields.

You have to use bitmasks in order for all endian cpus to work properly
here, so that needs to be fixed.

Oh, and if these values are in hardware, then the correct types also
need to be used (i.e. __u32 and __u64).

good catch!

greg k-h