On 19/01/21 18:20, Laszlo Ersek wrote:
I only have superficial comments:- if we're talking uint32_t, I'd kind of prefer UINT32_MAX to (-1), style-wise -- but feel free to ignore - we should print a uint32_t with ("%" PRIu32), not "%d" (again, only pedantry, but PRIu32 is widely used in qemu, AFAICS)
I would use %u, but yeah you're correct.
- OK, so get_image_size() returns an int64_t, which pci_add_option_rom() assigns to an "int" without any range checking.
This is pre-existing, but it should be fixed indeed.
Then we compare that int against the new uint32_t property... or else, on the other branch, we assign pow2ceil() -- a uint64_t -- to the new (uint32_t) property. - In pci_assign_dev_load_option_rom(), "st.st_size" (which is an off_t) is assigned to the new property... I find it hard to reason about whether this is safe. I'd suggest first cleaning up "int size" in pci_add_option_rom() -- use an int64_t, and maybe check it explicitly against some 32-bit limit? --, then introduce the new property as uint64_t. (Print it with PRIu64 then, I guess.)
ROM BARs cannot be 64-bit in size. There's just no room in configuration space for that. Anyway yes pci_add_option_rom() is iffy and I'll send v2.
Paolo
BTW there's another aspect of is_power_of_2(): it catches the zero value. If the power-of-two check is dropped, I wonder if a zero property value could cause a mess, so it might be prudent to catch that explicitly. (Precedent: see the (size == 0) check in pci_add_option_rom().) Anyway, feel free to ignore all of my points; I just find it hard to reason about the "logic" when the code is not obviously overflow-free in the first place. (I'm not implying there are overflows; the code may be free of overflows -- it's just not*obviously* so, to me anyway.)
