Re: [Xen-devel] [PATCH] fix potential null dereference

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 18 Jan 2018, Andrew Cooper wrote:
> On 18/01/2018 22:11, Stefano Stabellini wrote:
> > On Tue, 16 Jan 2018, Jan Beulich wrote:
> >>>>> On 15.01.18 at 19:51, <sstabellini@xxxxxxxxxx> wrote:
> >>> On Mon, 15 Jan 2018, Jan Beulich wrote:
> >>>>>>> On 13.01.18 at 07:21, <julien.grall@xxxxxxxxxx> wrote:
> >>>>> On 01/12/2018 11:40 PM, Stefano Stabellini wrote:
> >>>>>> handles can theoretically be NULL, check for it explicitly before
> >>>>>> dereferencing it.
> >>>>> I doubt handles could be NULL if LocateHandle succeed. This seems to be 
> >>>>> confirmed by the spec (Page 208 in UEFI spec 2.7).
> >>>>>
> >>>>> So I am not entirely convince we should add yet another check in the 
> >>>>> code. An ASSERT might be better.
> >>>> Indeed if there is a platform where NULL is coming back in the
> >>>> success case, that platform should be named as a justification
> >>>> in the commit message. Otherwise I don't see the value of this
> >>>> change.
> >>> Truthfully, it is mostly to silence Coverity. We can all appreciate when
> >>> static analysts cannot find defects in the code.
> >> So what does Coverity dislike here (the more that this is on a
> >> boot path, i.e. not exploitable by guests at all in the first place)?
> >> Merely the NULL pointer? What if the interface gave back a
> >> pointer with a value of 0x123456789abcdef?
> > Coverity complains "Dereferencing null pointer handles". The code path
> > to get to that point is the following, as explained by Coverity:
> >
> >   - handles is set to NULL at the beginning of efi_get_gop
> >   - status is not EFI_BUFFER_TOO_SMALL, AllocatePool is not called, handles 
> > is still NULL
> >   - later we call efi_bs->HandleProtocol(handles[i], "Dereferencing null 
> > pointer handles"
> >
> > Given that in the first call to LocateHandle we pass size == 0, by the
> > spec I don't see how it can return anything other than EFI_BUFFER_TOO_SMALL.
> >
> > In other words, if LocateHandle complies, then the code is correct. I'll
> > mark it as won't fix.
> 
> EFI is full of UB because of needing to pass and cast pointers with
> *(void **), and Coverity has a hard time following these (as a direct
> consequence of the compilers having a hard time following them).
> 
> As for the spec however, that is irrelevant.  The point is that there
> are billions of other integers which could be returned which aren't
> TOO_SMALL and OK, and without the EFI firmware source code to inspect,
> Coverity can't know.
> 
> Also, I'd like to see any EFI anywhere which successfully implements the
> spec...

I guess the question is: do we want to make Xen more resilient against
possible firmware malfunctioning? In this specific case, a bug in the
firmware causes Xen to crash at boot, while with this patch Xen
could try to run past it. There are pros and cons both ways, and I
don't have a strong opinion on the approach to take in general. However,
in this specific case, given the simplicity of this patch, I would be
tempted to make the change.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel