[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] pci: add romsize property
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH] pci: add romsize property |
Date: |
Fri, 22 Jan 2021 15:54:56 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
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.)