On Wed, Jul 01 2026, Tao Liu wrote: > Hi Pratyush, > > On Mon, Jun 29, 2026 at 11:13 PM Pratyush Yadav <[email protected]> wrote: >> >> +Cc IMA maintainers >> >> On Fri, Jun 26 2026, Baoquan He wrote: >> >> > Add kexec ML to CC. >> > >> > On Fri, Jun 26, 2026 at 7:46 AM Tao Liu <[email protected]> wrote: >> >> >> >> Kindly ping, any comments? >> >> >> >> Thanks, >> >> Tao Liu >> >> >> >> On Fri, May 29, 2026 at 3:35 PM Tao Liu <[email protected]> wrote: >> >> > >> >> > A NULL pointer reference issue is noticed in riscv's >> >> > machine_kexec_prepare, >> >> > where image->segment[i].buf might be NULL and copied unchecked. >> >> > >> >> > The NULL buf comes from security/integrity/ima/ima_kexec.c: >> >> > ima_add_kexec_buffer(), where kbuf is added by kexec_add_buffer(), >> >> > but kbuf.buffer is NULL. >> >> > >> >> > Fix this by simply adding a check before copy. >> >> > >> >> > Signed-off-by: Tao Liu <[email protected]> >> >> > --- >> >> > arch/riscv/kernel/machine_kexec.c | 3 +++ >> >> > 1 file changed, 3 insertions(+) >> >> > >> >> > diff --git a/arch/riscv/kernel/machine_kexec.c >> >> > b/arch/riscv/kernel/machine_kexec.c >> >> > index 2306ce3e5f22..d81d576f9cb5 100644 >> >> > --- a/arch/riscv/kernel/machine_kexec.c >> >> > +++ b/arch/riscv/kernel/machine_kexec.c >> >> > @@ -41,6 +41,9 @@ machine_kexec_prepare(struct kimage *image) >> >> > if (image->segment[i].memsz <= sizeof(fdt)) >> >> > continue; >> >> > >> >> > + if (image->segment[i].buf == NULL) >> >> > + continue; >> >> > + >> > >> > This is a good fix, maybe we can add code comments to explain it as >> > below, just for reference. >> > >> > /* >> > * Some segments (e.g. IMA) reserve space but have no buffer >> > * loaded yet. Skip them as they cannot contain an FDT. >> > */ >> > And is there any other place where the similar issue exists? e.g on >> > LoongArch? >> > >> > Other than above concerns, this patch looks good to me: >> > >> > Acked-by: Baoquan He <[email protected]> >> >> Yeah, the patch LGTM to me too. >> >> Acked-by: Pratyush Yadav <[email protected]> >> >> Although I think IMA can make this a bit easier to understand. First, in >> ima_add_kexec_buffer() it should set kbuf.buffer to NULL and kbuf.bufsz >> to 0 explicitly instead of using kexec_buffer and kexec_buffer_size >> which are initialized to NULL and 0, but never updated. Using the >> variables here adds an extra level of indirection. >> >> Also, perhaps we should add a comment in ima_add_kexec_buffer() about >> how this all works, since where the IMA buffer lives and where it gets >> updated it fairly complicated and took me some time to piece together. > > Thanks for your patch review and suggestions! I agree with your point > on the IMA part, I was confused by the code too, e.g in > ima_add_kexec_buffer(): > > void *kexec_buffer = NULL; > kbuf.buffer = kexec_buffer; > ret = kexec_add_buffer(&kbuf); > if (ret) { > pr_err("Error passing over kexec measurement buffer.\n"); > vfree(kexec_buffer); > return; > } > > Do we need to vfree(kexec_buffer)? When kexec_buffer is NULL and seems > never get updated. > > I'm not familiar with IMA code, maybe there is a reason which I'm unaware > of...
I think it is just leftover from refactors. kexec_buffer used to point to an actual buffer at some point and refactors to the function left them always NULL but never removed them. -- Regards, Pratyush Yadav
