EFI: Stash ROMs if they're not in the PCI BAR
Hello Matthew Garrett,
The patch dd5fc854de5f: "EFI: Stash ROMs if they're not in the PCI BAR" from Dec 5, 2012, leads to the following warning:
"arch/x86/boot/compressed/eboot.c:290 setup_efi_pci()
error: potentially dereferencing uninitialized 'pci_handle'."
254 static efi_status_t setup_efi_pci(struct boot_params *params) 255 {
256 efi_pci_io_protocol *pci;
257 efi_status_t status;
258 void **pci_handle;
259 efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
260 unsigned long nr_pci, size = 0;
261 int i;
262 struct setup_data *data;
263
264 data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
265
266 while (data && data->next)
267 data = (struct setup_data *)(unsigned long)data->next;
268
269 status = efi_call_phys5(sys_table->boottime->locate_handle,
270 EFI_LOCATE_BY_PROTOCOL, &pci_proto,
271 NULL, &size, pci_handle);
^^^^^^^^^^This hasn't been initialized yet.
272
273 if (status == EFI_BUFFER_TOO_SMALL) {
274 status = efi_call_phys3(sys_table->boottime->allocate_pool,
275 EFI_LOADER_DATA, size, &pci_handle);
276
regards,
dan carpenter
A message that is not in the archive (sent off-list or not yet received): 20130315082928.GA14520-dZEljifmRObu9KfB+GxooP8+0UxHXcjY@public.gmane.org
Quoted 26 lines · Dan Carpenter
Hello Matthew Garrett,
The patch
dd5fc854de5f: "EFI: Stash ROMs if they're not in the PCI BAR" from Dec 5, 2012, leads to the following warning:
"arch/x86/boot/compressed/eboot.c:290setup_efi_pci()
error: potentially dereferencing uninitialized 'pci_handle'."254 static
efi_status_tsetup_efi_pci(struct boot_params *params)255 {256 efi_pci_io_protocol *pci; 257 efi_status_t status; 258 void **pci_handle; 259 efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID; 260 unsigned long nr_pci, size = 0; 261 int i; 262 struct setup_data *data; 263 264 data = (struct setup_data *)(unsigned long)params->hdr.setup_data; 265 266 while (data && data->next) 267 data = (struct setup_data *)(unsigned long)data->next; 268 269 status = efi_call_phys5(sys_table->boottime->locate_handle, 270 EFI_LOCATE_BY_PROTOCOL, &pci_proto, 271 NULL, &size, pci_handle); ^^^^^^^^^^
Quoted 1 line
This hasn't been initialized yet.
True. It probably doesn't *matter* because the size is zero so the firmware is just going to ignore the pointer anyway. Although in that case I wonder why we couldn't have just passed NULL. Perhaps we expected that some firmware might do some validation on the pointer before getting to the size check?
Signature · 1 line
dwmw2
A message that is not in the archive (sent off-list or not yet received): 1363337872.2444.9.camel-Fexsq3y4057IgHVZqg5X0TlWvGAXklZc@public.gmane.org
Quoted 29 lines · David Woodhouse
Quoted 26 lines · Dan Carpenter
Hello Matthew Garrett,
The patch
dd5fc854de5f: "EFI: Stash ROMs if they're not in the PCI BAR" from Dec 5, 2012, leads to the following warning:
"arch/x86/boot/compressed/eboot.c:290setup_efi_pci()
error: potentially dereferencing uninitialized 'pci_handle'."254 static
efi_status_tsetup_efi_pci(struct boot_params *params)255 {256 efi_pci_io_protocol *pci; 257 efi_status_t status; 258 void **pci_handle; 259 efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID; 260 unsigned long nr_pci, size = 0; 261 int i; 262 struct setup_data *data; 263 264 data = (struct setup_data *)(unsigned long)params->hdr.setup_data; 265 266 while (data && data->next) 267 data = (struct setup_data *)(unsigned long)data->next; 268 269 status = efi_call_phys5(sys_table->boottime->locate_handle, 270 EFI_LOCATE_BY_PROTOCOL, &pci_proto, 271 NULL, &size, pci_handle); ^^^^^^^^^^Quoted 1 line
This hasn't been initialized yet.
Quoted 5 lines
True. It probably doesn't *matter* because the size is zero so the firmware is just going to ignore the pointer anyway. Although in that case I wonder why we couldn't have just passed NULL. Perhaps we expected that some firmware might do some validation on the pointer before getting to the size check?
I doubt that the firmware checks the validity of pci_handle when size is zero, and I agree it's worth passing NULL to silence the warning (which is also more explicit that just initialising pci_handle), unless Matthew knows of a reason we shouldn't do that?
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
A message that is not in the archive (sent off-list or not yet received): 1363606563.15011.333.camel-ZqTwcBeJ+wsBof6jY8KHXm7IUlhRatedral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Quoted 11 lines · Matt Fleming
Quoted 5 lines · David Woodhouse
True. It probably doesn't *matter* because the size is zero so the firmware is just going to ignore the pointer anyway. Although in that case I wonder why we couldn't have just passed NULL. Perhaps we expected that some firmware might do some validation on the pointer before getting to the size check?
I doubt that the firmware checks the validity of
pci_handlewhen size is zero, and I agree it's worth passing NULL to silence the warning (which is also more explicit that just initialisingpci_handle), unless Matthew knows of a reason we shouldn't do that?
No reason I can think of, and any failure will be pretty immediately obvious.
Signature · 1 line
Matthew Garrett | [email protected]
A message that is not in the archive (sent off-list or not yet received): 1363622520.2553.8.camel-tCUS0Eywp2Pehftex57rkxo58HMYffqeLoYNBG0abjxeoWH0uzbU5w@public.gmane.org
Quoted 8 lines · Matthew Garrett
Quoted 7 lines · Matt Fleming
Quoted 5 lines · David Woodhouse
True. It probably doesn't *matter* because the size is zero so the firmware is just going to ignore the pointer anyway. Although in that case I wonder why we couldn't have just passed NULL. Perhaps we expected that some firmware might do some validation on the pointer before getting to the size check?
Quoted 7 lines
Quoted 4 lines
I doubt that the firmware checks the validity of
pci_handlewhen size is zero, and I agree it's worth passing NULL to silence the warning (which is also more explicit that just initialisingpci_handle), unless Matthew knows of a reason we shouldn't do that?No reason I can think of, and any failure will be pretty immediately obvious.
Anyone want to submit a patch?
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
A message that is not in the archive (sent off-list or not yet received): 514976F3.2-HNK1S37rvNbeXh+fF434Mdi2O/[email protected]
Quoted 14 lines · Matt Fleming
Quoted 13 lines · Matthew Garrett
Quoted 11 lines · Matt Fleming
Quoted 5 lines · David Woodhouse
True. It probably doesn't *matter* because the size is zero so the firmware is just going to ignore the pointer anyway. Although in that case I wonder why we couldn't have just passed NULL. Perhaps we expected that some firmware might do some validation on the pointer before getting to the size check?
I doubt that the firmware checks the validity of
pci_handlewhen size is zero, and I agree it's worth passing NULL to silence the warning (which is also more explicit that just initialisingpci_handle), unless Matthew knows of a reason we shouldn't do that?
Quoted 5 lines
Quoted 2 lines
No reason I can think of, and any failure will be pretty immediately obvious.
Anyone want to submit a patch?
Sure. I'll send one tomorrow.
regards,
dan carpenter
Quoted 8 lines · Matthew Garrett
Quoted 7 lines · Matt Fleming
Quoted 5 lines · David Woodhouse
True. It probably doesn't *matter* because the size is zero so the firmware is just going to ignore the pointer anyway. Although in that case I wonder why we couldn't have just passed NULL. Perhaps we expected that some firmware might do some validation on the pointer before getting to the size check?
Quoted 7 lines
Quoted 4 lines
I doubt that the firmware checks the validity of
pci_handlewhen size is zero, and I agree it's worth passing NULL to silence the warning (which is also more explicit that just initialisingpci_handle), unless Matthew knows of a reason we shouldn't do that?No reason I can think of, and any failure will be pretty immediately obvious.
I agree with Matthew that I can think of no reason why a sane implementation of EFI would ever actually dereference the pointer in that case¹.
But there's an important difference in the way I phrased it...
I can't see why a *sane* implementation of EFI would do a lot of things. Many of which have actually been seen in the field, much to our distress.
So I'm starting to to think "hm, maybe we should give it a valid pointer anyway, just in case".
Remember: Any sufficiently advanced incompetence is indistinguishable from malice.
Assuming malice on the part of firmware authors is a fairly good way to model their potential behaviour. So I'm perfectly prepared that someone would ship an implementation which does *no* input checking, ever. Except for gratuitously checking that particular pointer for NULL and bailing out with an 'invalid parameters' error, even if the length was zero.
Signature · 4 lines
dwmw2 ¹ Although I haven't actually checked the spec to see if the pointer is required to be valid even when length is zero.