On Wed, 29 Apr 2026 at 19:35, Harsh Prateek Bora <[email protected]> wrote:
>
> From: Aditya Gupta <[email protected]>
>
> Set the "Thread Register State Entry Size" that is required by firmware
> (OPAL), to know size of memory to allocate to capture CPU state, in the
> event of a crash
>
> Reviewed-by: Hari Bathini <[email protected]>
> Reviewed-by: Sourabh Jain <[email protected]>
> Signed-off-by: Aditya Gupta <[email protected]>
> Tested-by: Shivang Upadhyay <[email protected]>
> Link:
> https://lore.kernel.org/qemu-devel/[email protected]
> Signed-off-by: Harsh Prateek Bora <[email protected]>
Hi; Coverity points out an issue with this change (CID 1658041):
> static void pnv_reset(MachineState *machine, ResetType type)
> {
> + PnvMachineState *pnv = PNV_MACHINE(machine);
> void *fdt;
>
> qemu_devices_reset(type);
>
> + if (!pnv->mpipl_state.is_next_boot_mpipl) {
> + /*
> + * Set the "Thread Register State Entry Size", so that firmware can
> + * allocate enough memory to capture CPU state in the event of a
> + * crash
> + */
> +
> + MpiplProcDumpArea proc_area;
Here we don't initialize the struct...
> +
> + proc_area.version = PROC_DUMP_AREA_VERSION_P9;
> + proc_area.thread_size = cpu_to_be32(sizeof(MpiplPreservedCPUState));
> +
> + /* These are to be allocated & assigned by the firmware */
> + proc_area.alloc_addr = 0;
> + proc_area.alloc_size = 0;
> +
> + /* These get assigned after crash, when QEMU preserves the registers
> */
> + proc_area.dest_addr = 0;
> + proc_area.act_size = 0;
...and here we don't fill in all the fields; we don't set
the reserved, reserved2 or reserved3 fields to anything.
This means that we will write data to the guest which is
potentially random host data from the stack.
I think I'd suggest fixing this by initializing the struct in
one go, like this:
MpiplProcDumpArea proc_area = {
.version = PROC_DUMP_AREA_VERSION_P9,
.thread_size = cpu_to_be32(sizeof(MpiplPreservedCPUState)),
[set alloc_addr = 0 etc here if you like, or rely on
the "fields not listed are zero-inited"]
};
(In fact because we use -ftrivial-auto-var-init=zero the compiler
will zero init this for us anyway, but we can make Coverity
happy anyway.)
> +
> + cpu_physical_memory_write(PROC_DUMP_AREA_OFF, &proc_area,
> + sizeof(proc_area));
> + }
> +
> fdt = machine->fdt;
> cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
> }
thanks
-- PMM
