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. Regards, Simon
