[Top][All Lists]

[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.


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.)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]