Hi Simon, On Tue, Oct 13, 2026 at 5:25 AM Simon Glass <[email protected]> wrote: > > Hi Anirudh, > > On 2026-10-03T20:07:54, Anirudh Srinivasan > <[email protected]> wrote: > > bios_emulator: don't free a caller-supplied BIOS image in biosemu_run > > Please say where this is actually hit. pci_run_vga_bios() passes the > image from pci_rom_load(). On x86 that image is at > PCI_VGA_RAM_IMAGE_START (0xc0000), so the existing 0xc0000 check > avoids the problem. On other architectures the image is malloc()ed and > pci_run_vga_bios() frees it again on exit, which is the double free. A > Fixes: tag would also help, if you can find the commit that started > passing the image in. > > Whilte you are here: the subject is 69 characters. Please can you > shorten it, e.g. 'bios_emulator: Don't free a caller's BIOS image'. In > the body, please write biosemu_run() and PCI_postController() with > brackets. Also 'i.e' should be 'i.e.' > > > > > biosemu_run can be called with a BIOS image supplied by the caller > > (argument bios_rom is not NULL), or with no image so that it makes its > > own copy from the card's ROM (argument bios_rom is NULL). > > > > It subsequently calls PCI_postController, where the BIOS image it runs > > is stored in vga_info->BIOSImage. In the first case that pointer is the > > caller's buffer; in the second it is a buffer biosemu_run itself > > allocated. > > > > The clean_up path in biosemu_run always frees vga_info->BIOSImage. That > > is right for the second case, but in the first case it frees memory the > > caller still owns. This can cause a double free when the caller tries > > freeing this memory again. > > > > Only free vga_info->BIOSImage when biosemu_run made the copy itself (i.e > > bios_rom is NULL) > > > > Signed-off-by: Anirudh Srinivasan <[email protected]> > > > > drivers/bios_emulator/atibios.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > diff --git a/drivers/bios_emulator/atibios.c > > b/drivers/bios_emulator/atibios.c > > @@ -509,7 +509,7 @@ int biosemu_run(struct udevice *pcidev, uchar > > *bios_rom, int bios_len, > > - if (vga_info->BIOSImage && > > + if (!bios_rom && vga_info->BIOSImage && > > (ulong)(vga_info->BIOSImage) != 0xc0000) > > The fix looks right, but with it the other two conditions do nothing. > When bios_rom is NULL, BIOSImage is always the buffer > PCI_postController() allocated, so it can never be 0xc0000; that check > was only a workaround for the caller-supplied case on x86. free(NULL) > is also fine, although PCI_postController() returns false before it > sets BIOSImage to NULL anyway. Please can you simplify this to: > > if (!bios_rom) > free(vga_info->BIOSImage); > > That also makes the ownership rule clear. > > BTW, PCI_postController() leaks copy_of_bios and vga_info when it > fails after the malloc() (e.g. on an invalid ROM signature), but that > predates this patch and can be fixed separately.
Thanks for the comments, will make the changes to the commit message and drop the extra conditions checked before freeing the biosimage. > > Regards, > Simon
