[PATCHv2 2/3] kexec: Introduce UKI image parser

[Pingfan Liu]

Hi Simon,

Thanks for your review.

On Mon, Oct 21, 2024 at 6:16 PM Simon Horman <horms at kernel.org> wrote:
>
> On Wed, Oct 16, 2024 at 07:34:14PM +0800, Pingfan Liu wrote:
> > A UKI image is a PE file that consists of several sections, typically
> > including: .text, .data, .linux, .initrd, .cmdline, and others.
> >
> > The kernel image is stored in the .linux section, which is one of the
> > formats currently recognized by kexec-tools. Therefore, the UKI parser
> > can be used to strip away the UKI layer, allowing the other parser to
> > continue the procession of the kernel image.
> >
> > Signed-off-by: Pingfan Liu <piliu at redhat.com>
>
> ...
>
> > diff --git a/kexec/kexec-uki.c b/kexec/kexec-uki.c
>
> ...
>
> > +int uki_image_probe(const char *file_buf, off_t buf_sz)
> > +{
> > +     struct pe_hdr *pe_hdr;
> > +     struct pe32plus_opt_hdr *opt_hdr;
> > +     struct section_header *sect_hdr;
> > +     int pe_hdr_offset, section_nr, linux_sz = -1;
> > +     char *pe_part_buf, *linux_src;
> > +     char *initrd_fname = NULL;
> > +     int initrd_fd = -1;
> > +
> > +     pe_hdr_offset = get_pehdr_offset(file_buf);
> > +     pe_part_buf = (char *)file_buf + pe_hdr_offset;
> > +     pe_hdr = (struct pe_hdr *)pe_part_buf;
> > +     if (pe_hdr->opt_hdr_size == 0) {
> > +             printf("ERR: optional header is missing\n");
> > +             return -1;
> > +     }
> > +     section_nr = pe_hdr->sections;
> > +     opt_hdr = (struct pe32plus_opt_hdr *)(pe_part_buf + sizeof(struct pe_hdr));
> > +     sect_hdr = (struct section_header *)((char *)opt_hdr + pe_hdr->opt_hdr_size);
> > +
> > +     for (int i = 0; i < section_nr; i++) {
> > +             if (!strcmp(sect_hdr->name, UKI_LINUX_SECTION)) {
> > +                     /* data_addr is relative to the whole file */
> > +                     linux_src = (char *)file_buf + sect_hdr->data_addr;
> > +                     linux_sz = sect_hdr->raw_data_size;
> > +
> > +             } else if (!strcmp(sect_hdr->name, UKI_INITRD_SECTION)) {
> > +                     if (!(initrd_fname = strdup(FILENAME_UKI_INITRD))) {
> > +                             dbgprintf("%s: Can't duplicate strings\n", __func__);
> > +                             goto next;
> > +                     }
> > +
> > +                     if ((initrd_fd = mkstemp(initrd_fname)) < 0) {
> > +                             dbgprintf("%s: Can't open file %s\n", __func__, initrd_fname);
> > +                             goto next;
> > +                     }
> > +
> > +                     if (write(initrd_fd, (char *)file_buf + sect_hdr->data_addr,
> > +                                     sect_hdr->raw_data_size) != sect_hdr->raw_data_size) {
> > +                             dbgprintf("%s: Can't write the compressed file %s\n",
> > +                                             __func__, initrd_fname);
> > +                             goto next;
>
> I plan to apply this series, but initrd_fd appears to be leaked here.
> Could you send a follow-up patch to address that? Perhaps like this:
>
I have sent out a dedicated v3 "[PATCHv3 2/3] kexec: Introduce UKI
image parser" to fix this leak issue.

>                         err = write(...);
>                         close(initrd_fd)

My fix duplicates the close(initrd_fd) in each branch. Since initrd_fd
is the only reference to mkstemp(initrd_fname), I think if closing it,
the temp file may disappear before implicit_initrd_fd=
open(initrd_fname, O_RDONLY).


Best Regards,

Pingfan

>                         if (err != sect_hdr->raw_data_size) {
>                                 ...
>                         } else {
>                                 ...
>                         }
>
> Thanks!
>
> > +                     } else {
> > +                             implicit_initrd_fd = open(initrd_fname, O_RDONLY);
> > +                             close(initrd_fd);
> > +                     }
> > +             }
> > +next:
> > +             sect_hdr++;
> > +     }
>
> ...
>