re: EFI: Stash ROMs if they're not in the PCI BAR

re: EFI: Stash ROMs if they're not in the PCI BAR

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

Re: EFI: Stash ROMs if they're not in the PCI BAR

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On Fri, 2013-03-15 at 11:29 +0300, Dan Carpenter wrote:
> 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.

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?

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: EFI: Stash ROMs if they're not in the PCI BAR

[Matt Fleming]

On Fri, 2013-03-15 at 08:57 +0000, David Woodhouse wrote:
> On Fri, 2013-03-15 at 11:29 +0300, Dan Carpenter wrote:
> > 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.
> 
> 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?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: EFI: Stash ROMs if they're not in the PCI BAR

[Matthew Garrett]

On Mon, 2013-03-18 at 11:36 +0000, Matt Fleming wrote:
> On Fri, 2013-03-15 at 08:57 +0000, David Woodhouse wrote:
> > 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?

No reason I can think of, and any failure will be pretty immediately
obvious.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: EFI: Stash ROMs if they're not in the PCI BAR

[Matt Fleming]

On 03/18/2013 04:02 PM, Matthew Garrett wrote:
> On Mon, 2013-03-18 at 11:36 +0000, Matt Fleming wrote:
>> On Fri, 2013-03-15 at 08:57 +0000, David Woodhouse wrote:
>>> 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?
> 
> No reason I can think of, and any failure will be pretty immediately
> obvious.

Anyone want to submit a patch?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: EFI: Stash ROMs if they're not in the PCI BAR

[Dan Carpenter]

On Wed, Mar 20, 2013 at 08:44:35AM +0000, Matt Fleming wrote:
> On 03/18/2013 04:02 PM, Matthew Garrett wrote:
> > On Mon, 2013-03-18 at 11:36 +0000, Matt Fleming wrote:
> >> On Fri, 2013-03-15 at 08:57 +0000, David Woodhouse wrote:
> >>> 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?
> > 
> > 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

Re: EFI: Stash ROMs if they're not in the PCI BAR

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

On Mon, 2013-03-18 at 16:02 +0000, Matthew Garrett wrote:
> On Mon, 2013-03-18 at 11:36 +0000, Matt Fleming wrote:
> > On Fri, 2013-03-15 at 08:57 +0000, David Woodhouse wrote:
> > > 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?
> 
> 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.

-- 
dwmw2

¹ Although I haven't actually checked the spec to see if the pointer is
  required to be valid even when length is zero.



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]