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

[sashiko-bot@kernel.org]

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?

> +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)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-rpcit_trcfh_upstream-v1-0-8a2718cfb90b@linux.ibm.com?part=1