Re: [PATCH 1/3] s390/pci: Adjust alignment in insn trace

[Gerd Bayer <gbayer@linux.ibm.com>]

On Wed, 2026-10-07 at 11:14 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] s390/pci: info leak of kernel stack memory in trace buffer due to unnamed bit-fields
> --
> 
> commit 9eddce7825c1b38a4a78855744726217e301528b
> Author: Gerd Bayer <gbayer@linux.ibm.com>
> 
> s390/pci: Adjust alignment in insn trace
> 
> This patch adjusts the alignment of fields in the pci_error instruction
> trace. It drops the packed attribute on struct zpci_err_insn_data and
> adds unnamed bit-fields to explicitly pad the structure to 32 bytes for
> easier human readability in hex dumps.
> 
> > diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
> > index 35ceb1bea1c61..2ba787e2304b5 100644
> > --- a/arch/s390/pci/pci_insn.c
> > +++ b/arch/s390/pci/pci_insn.c
> > @@ -18,11 +18,14 @@
> >  #include <asm/asm.h>
> >  
> >  #define ZPCI_INSN_BUSY_DELAY	1	/* 1 microsecond */
> > +#define ZPCI_ERR_INSN_DATA_SIZE 32
> >  
> >  struct zpci_err_insn_data {
> >  	u8 insn;
> >  	u8 cc;
> >  	u8 status;
> > +	u8: 8;
> > +	u32: 32;
> >  	union {
> >  		struct {
> >  			u64 req;
> > @@ -33,7 +36,10 @@ struct zpci_err_insn_data {
> >  			u64 len;
> >  		};
> >  	};
> > -} __packed;
> > +	u64: 64;
> > +};
> > +
> 
> [Severity: Medium]
> Does this change unintentionally leak kernel stack memory into the s390dbf
> debug trace ring buffer?
> 
> By using unnamed bit-fields for padding, these fields are explicitly
> excluded from zero-initialization when using a designated initializer.
> 
> When zpci_err_insn_req() in arch/s390/pci/pci_insn.c initializes this
> structure on the stack without an explicit memset(), the unnamed padding
> bytes retain their uninitialized stack memory:
> 
> static inline void zpci_err_insn_req(...)
> {
>         struct zpci_err_insn_data data = {
>                 .insn = insn, .cc = cc, .status = status,
>                 .req = req, .offset = offset};
> 
>         zpci_err_hex_level(lvl, &data, sizeof(data));
> }
> 
> The subsequent call to zpci_err_hex_level() will copy the entire 32-byte
> structure to the trace buffer, exposing 13 bytes of uninitialized stack
> data.
> 
> Will this also defeat the goal of having clean traces for humans, since
> the hex dumps will contain random stack data instead of zeroes?


Alright, according to some research [1][2] unnamed bit-fields are not
guaranteed to be zero-initialized. I'll provide dummy-names for the
padding fields in the next iteration.

> 
> > +static_assert(sizeof(struct zpci_err_insn_data) <= ZPCI_ERR_INSN_DATA_SIZE);
> >  
> >  static inline void zpci_err_insn_req(int lvl, u8 insn, u8 cc, u8 status,
> >  				     u64 req, u64 offset)

[1] - https://stackoverflow.com/questions/56216243/how-do-bit-fields-interplay-with-bits-padding-in-c
[2] - https://news.ycombinator.com/item?id=43792948