[PATCH] x86/EFI: additional checks in efi_bgrt_init()

[PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

Header length should be validated for all ACPI tables before accessing
any non-header field.

The valid flags should also be check, as with it clear there's no point
in trying to go through the rest of the code (and there's no guarantee
that the other table contents are valid/consistent in that case).

Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>

---
 arch/x86/platform/efi/efi-bgrt.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 3.7-rc4/arch/x86/platform/efi/efi-bgrt.c
+++ 3.7-rc4-x86-EFI-BGRT-checks/arch/x86/platform/efi/efi-bgrt.c
@@ -39,7 +39,9 @@ void efi_bgrt_init(void)
 	if (ACPI_FAILURE(status))
 		return;
 
-	if (bgrt_tab->version != 1)
+	if (bgrt_tab->header.length < sizeof(*bgrt_tab))
+		return;
+	if (bgrt_tab->version != 1 || !(bgrt_tab->status & 1))
 		return;
 	if (bgrt_tab->image_type != 0 || !bgrt_tab->image_address)
 		return;

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Josh Triplett]

On Mon, Nov 05, 2012 at 03:26:41PM +0000, Jan Beulich wrote:
> Header length should be validated for all ACPI tables before accessing
> any non-header field.
> 
> The valid flags should also be check, as with it clear there's no point
> in trying to go through the rest of the code (and there's no guarantee
> that the other table contents are valid/consistent in that case).
> 
> Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>

The length check seems reasonable.  However, Matthew Garrett (already
CCed) previously suggested to me that this code should not check the
"valid" bit, and should instead present the information to userspace if
otherwise valid (such as having image_address != 0).  Matthew?

Out of curiosity, did you encounter a system that requires this patch?

- Josh Triplett

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Matthew Garrett]

On Mon, Nov 05, 2012 at 10:37:52AM -0800, Josh Triplett wrote:
> On Mon, Nov 05, 2012 at 03:26:41PM +0000, Jan Beulich wrote:
> > Header length should be validated for all ACPI tables before accessing
> > any non-header field.
> > 
> > The valid flags should also be check, as with it clear there's no point
> > in trying to go through the rest of the code (and there's no guarantee
> > that the other table contents are valid/consistent in that case).
> > 
> > Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> 
> The length check seems reasonable.  However, Matthew Garrett (already
> CCed) previously suggested to me that this code should not check the
> "valid" bit, and should instead present the information to userspace if
> otherwise valid (such as having image_address != 0).  Matthew?

Yeah, my interpretation of the spec is that "valid" indicates whether or 
not the contents represent what's currently on the screen, not whether 
or not the contents can be interpreted for other reasons.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Josh Triplett]

On Mon, Nov 05, 2012 at 06:43:46PM +0000, Matthew Garrett wrote:
> On Mon, Nov 05, 2012 at 10:37:52AM -0800, Josh Triplett wrote:
> > On Mon, Nov 05, 2012 at 03:26:41PM +0000, Jan Beulich wrote:
> > > Header length should be validated for all ACPI tables before accessing
> > > any non-header field.
> > > 
> > > The valid flags should also be check, as with it clear there's no point
> > > in trying to go through the rest of the code (and there's no guarantee
> > > that the other table contents are valid/consistent in that case).
> > > 
> > > Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> > 
> > The length check seems reasonable.  However, Matthew Garrett (already
> > CCed) previously suggested to me that this code should not check the
> > "valid" bit, and should instead present the information to userspace if
> > otherwise valid (such as having image_address != 0).  Matthew?
> 
> Yeah, my interpretation of the spec is that "valid" indicates whether or 
> not the contents represent what's currently on the screen, not whether 
> or not the contents can be interpreted for other reasons.

Given that, in the absence of a real BIOS that interprets the spec
differently than that, it sounds like we should drop the check for the
valid bit.

Jan, have you seen a real BIOS which disagrees with the above
interpretation?  If not, can you resubmit the patch with just the length
check?

- Josh Triplett

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

>>> On 05.11.12 at 20:00, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> On Mon, Nov 05, 2012 at 06:43:46PM +0000, Matthew Garrett wrote:
>> On Mon, Nov 05, 2012 at 10:37:52AM -0800, Josh Triplett wrote:
>> > On Mon, Nov 05, 2012 at 03:26:41PM +0000, Jan Beulich wrote:
>> > > Header length should be validated for all ACPI tables before accessing
>> > > any non-header field.
>> > > 
>> > > The valid flags should also be check, as with it clear there's no point
>> > > in trying to go through the rest of the code (and there's no guarantee
>> > > that the other table contents are valid/consistent in that case).
>> > > 
>> > > Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
>> > 
>> > The length check seems reasonable.  However, Matthew Garrett (already
>> > CCed) previously suggested to me that this code should not check the
>> > "valid" bit, and should instead present the information to userspace if
>> > otherwise valid (such as having image_address != 0).  Matthew?
>> 
>> Yeah, my interpretation of the spec is that "valid" indicates whether or 
>> not the contents represent what's currently on the screen, not whether 
>> or not the contents can be interpreted for other reasons.

That would be nice to get clarified.

> Given that, in the absence of a real BIOS that interprets the spec
> differently than that, it sounds like we should drop the check for the
> valid bit.
> 
> Jan, have you seen a real BIOS which disagrees with the above
> interpretation?

I would first have to check the rest of the data when the "valid"
bit is clear on the two system I have (out of which one may not
support BGRT at all).

The question here is how to properly invalidate that data then:
So far I was assuming that clearing the valid bit would be the way
to go, as the specification says nothing on e.g. the image address
being zero having any specific meaning. I need to do that in Xen,
at least for the time being, as I'm not inclined to postpone
indefinitely the use of boot services memory for normal memory
allocations (which we would have to do if we wanted Dom0 to
be able to access the image pointed to here).

I was quite bad a design decision to allow (and even suggest)
the image to live in boot services memory - one can't generally
expect the ACPI parser to become available in an OS before
setting up memory allocation. To Linux this already has turned
out to be a problem, on Xen dealing with this would turn out
even more cumbersome.

> If not, can you resubmit the patch with just the length check?

Sure - once clarified.

Jan

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Josh Triplett]

On Tue, Nov 06, 2012 at 08:57:06AM +0000, Jan Beulich wrote:
> The question here is how to properly invalidate that data then:
> So far I was assuming that clearing the valid bit would be the way
> to go, as the specification says nothing on e.g. the image address
> being zero having any specific meaning. I need to do that in Xen,
> at least for the time being, as I'm not inclined to postpone
> indefinitely the use of boot services memory for normal memory
> allocations (which we would have to do if we wanted Dom0 to
> be able to access the image pointed to here).

This doesn't postpone the use of boot services memory indefinitely; the
BGRT driver copies the image out, and the kernel then frees boot
services memory.  That does introduce a delay in the use of boot
services memory, but not an indefinite one.

> I was quite bad a design decision to allow (and even suggest)
> the image to live in boot services memory - one can't generally
> expect the ACPI parser to become available in an OS before
> setting up memory allocation. To Linux this already has turned
> out to be a problem, on Xen dealing with this would turn out
> even more cumbersome.

What problems has this caused?

- Josh Triplett

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

>>> On 06.11.12 at 13:55, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> On Tue, Nov 06, 2012 at 08:57:06AM +0000, Jan Beulich wrote:
>> The question here is how to properly invalidate that data then:
>> So far I was assuming that clearing the valid bit would be the way
>> to go, as the specification says nothing on e.g. the image address
>> being zero having any specific meaning. I need to do that in Xen,
>> at least for the time being, as I'm not inclined to postpone
>> indefinitely the use of boot services memory for normal memory
>> allocations (which we would have to do if we wanted Dom0 to
>> be able to access the image pointed to here).
> 
> This doesn't postpone the use of boot services memory indefinitely; the
> BGRT driver copies the image out, and the kernel then frees boot
> services memory.  That does introduce a delay in the use of boot
> services memory, but not an indefinite one.

If and when Dom0 tells the hypervisor that it's done using that
data is unknown from the hypervisor perspective, so for Xen
this _is_ indefinitely.

>> I was quite bad a design decision to allow (and even suggest)
>> the image to live in boot services memory - one can't generally
>> expect the ACPI parser to become available in an OS before
>> setting up memory allocation. To Linux this already has turned
>> out to be a problem, on Xen dealing with this would turn out
>> even more cumbersome.
> 
> What problems has this caused?

None so far - I'm trying to prevent any from occurring.

Or if you're referring to my use of the term "problems" above - I
was referring to the fact that for this very reason the treatment
of boot services memory had to be changed in Linux.

Jan

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Josh Triplett]

On Tue, Nov 06, 2012 at 01:37:10PM +0000, Jan Beulich wrote:
> >>> On 06.11.12 at 13:55, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > On Tue, Nov 06, 2012 at 08:57:06AM +0000, Jan Beulich wrote:
> >> The question here is how to properly invalidate that data then:
> >> So far I was assuming that clearing the valid bit would be the way
> >> to go, as the specification says nothing on e.g. the image address
> >> being zero having any specific meaning. I need to do that in Xen,
> >> at least for the time being, as I'm not inclined to postpone
> >> indefinitely the use of boot services memory for normal memory
> >> allocations (which we would have to do if we wanted Dom0 to
> >> be able to access the image pointed to here).
> > 
> > This doesn't postpone the use of boot services memory indefinitely; the
> > BGRT driver copies the image out, and the kernel then frees boot
> > services memory.  That does introduce a delay in the use of boot
> > services memory, but not an indefinite one.
> 
> If and when Dom0 tells the hypervisor that it's done using that
> data is unknown from the hypervisor perspective, so for Xen
> this _is_ indefinitely.

Ah, I see.

> >> I was quite bad a design decision to allow (and even suggest)
> >> the image to live in boot services memory - one can't generally
> >> expect the ACPI parser to become available in an OS before
> >> setting up memory allocation. To Linux this already has turned
> >> out to be a problem, on Xen dealing with this would turn out
> >> even more cumbersome.
> > 
> > What problems has this caused?
> 
> None so far - I'm trying to prevent any from occurring.
> 
> Or if you're referring to my use of the term "problems" above - I
> was referring to the fact that for this very reason the treatment
> of boot services memory had to be changed in Linux.

Oh, I definitely agree about the poor design decision of putting the
image in boot services memory.  I had specifically wondered what
problems had occurred in Linux and Xen due to needing the ACPI parser
before freeing boot services memory.

- Josh Triplett

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

>>> On 06.11.12 at 09:57, Jan Beulich wrote:
>>>> On 05.11.12 at 20:00, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > On Mon, Nov 05, 2012 at 06:43:46PM +0000, Matthew Garrett wrote:
> >> On Mon, Nov 05, 2012 at 10:37:52AM -0800, Josh Triplett wrote:
> >> > On Mon, Nov 05, 2012 at 03:26:41PM +0000, Jan Beulich wrote:
> >> > > Header length should be validated for all ACPI tables before accessing
> >> > > any non-header field.
> >> > > 
> >> > > The valid flags should also be check, as with it clear there's no point
> >> > > in trying to go through the rest of the code (and there's no guarantee
> >> > > that the other table contents are valid/consistent in that case).
> >> > > 
> >> > > Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> >> > 
> >> > The length check seems reasonable.  However, Matthew Garrett (already
> >> > CCed) previously suggested to me that this code should not check the
> >> > "valid" bit, and should instead present the information to userspace if
> >> > otherwise valid (such as having image_address != 0).  Matthew?
> >> 
> >> Yeah, my interpretation of the spec is that "valid" indicates whether or 
> >> not the contents represent what's currently on the screen, not whether 
> >> or not the contents can be interpreted for other reasons.
> 
> That would be nice to get clarified.
> 
> > Given that, in the absence of a real BIOS that interprets the spec
> > differently than that, it sounds like we should drop the check for the
> > valid bit.
> > 
> > Jan, have you seen a real BIOS which disagrees with the above
> > interpretation?
> 
> I would first have to check the rest of the data when the "valid"
> bit is clear on the two system I have (out of which one may not
> support BGRT at all).

On my system, the table points to EfiReservedMemoryType
and appears to have valid data despite the valid flag being
clear. But before encoding the invalidation into Xen in a form
other than clearing the valid bit, I'd really like to get
confirmation that e.g. clearing the image address is indeed a
"proper" thing to do. If no-one among you can "officially" tell
what the intentions here are, would you know who can?

Jan

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Matthew Garrett]

On Wed, Nov 07, 2012 at 02:48:52PM +0000, Jan Beulich wrote:

> On my system, the table points to EfiReservedMemoryType
> and appears to have valid data despite the valid flag being
> clear. But before encoding the invalidation into Xen in a form
> other than clearing the valid bit, I'd really like to get
> confirmation that e.g. clearing the image address is indeed a
> "proper" thing to do. If no-one among you can "officially" tell
> what the intentions here are, would you know who can?

If you don't want to provide it at all, just don't provide a BGRT. 
Microsoft wrote that bit of the ACPI spec, so you'd probably need to ask 
them what their intentions were.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org