The XEN_FW_EFI_MEM_INFO platform op has very surprising behavior: it
only sets info->mem.size if the initial value was *larger* than the size
of the memory region.  This is not particularly useful and cost me most
of a day of debugging.  It also has some integer overflow problems,
though as the data comes from dom0 or the firmware (both of which are
trusted) these are not security issues.

Fix both of these problems by unconditionally setting the memory region
size and by computing it in a way that is immune to integer overflow.
The new code is slightly longer, but it is much easier to understand and
use.
---
 xen/common/efi/runtime.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index a8fc2b99ae098d74af1978bdf58212eb99cce70f..a086850c9b0bbb6e4dd3ccca647c09d346f87c55 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -269,19 +269,21 @@
     case XEN_FW_EFI_MEM_INFO:
         for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
         {
+            uint64_t len;
             EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
-            u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+
+            if ( desc->NumberOfPages > (UINT64_MAX >> EFI_PAGE_SHIFT) )
+                len = UINT64_MAX;
+            else
+                len = desc->NumberOfPages << EFI_PAGE_SHIFT;
 
             if ( info->mem.addr >= desc->PhysicalStart &&
-                 info->mem.addr < desc->PhysicalStart + len )
+                 info->mem.addr - desc->PhysicalStart < len )
             {
                 info->mem.type = desc->Type;
                 info->mem.attr = desc->Attribute;
-                if ( info->mem.addr + info->mem.size < info->mem.addr ||
-                     info->mem.addr + info->mem.size >
-                     desc->PhysicalStart + len )
-                    info->mem.size = desc->PhysicalStart + len -
-                                     info->mem.addr;
+                info->mem.size = len - (info->mem.addr - desc->PhysicalStart);
+
                 return 0;
             }
         }
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

On 24.08.2022 23:04, Demi Marie Obenour wrote:
> The XEN_FW_EFI_MEM_INFO platform op has very surprising behavior: it
> only sets info->mem.size if the initial value was *larger* than the size
> of the memory region.

And intentionally so - the caller didn't ask for any bigger region,
after all.

>  This is not particularly useful and cost me most
> of a day of debugging.  It also has some integer overflow problems,
> though as the data comes from dom0 or the firmware (both of which are
> trusted) these are not security issues.

I'm afraid we're trusting the firmware in this regard elsewhere as
well. So if there was a need to change that, I guess it would need
changing everywhere, not just here. But we trust the E820 map as
well, when on non-EFI platforms, so I don't see why we would need
to change that. In any event such would want to be a separate
change imo.

> Fix both of these problems by unconditionally setting the memory region
> size

If you were to report a larger ending address, why would you not also
report a smaller starting address?

But before you go that route - I don't think we can change the API
now that it has been in use this way for many years. If a "give me
the full enclosing range" variant is wanted, it will need to be
fully separate.

Jan

> and by computing it in a way that is immune to integer overflow.
> The new code is slightly longer, but it is much easier to understand and
> use.

On Thu, Aug 25, 2022 at 09:59:56AM +0200, Jan Beulich wrote:
> On 24.08.2022 23:04, Demi Marie Obenour wrote:
> > The XEN_FW_EFI_MEM_INFO platform op has very surprising behavior: it
> > only sets info->mem.size if the initial value was *larger* than the size
> > of the memory region.
> 
> And intentionally so - the caller didn't ask for any bigger region,
> after all.

That needs to be documented, then.  I thought it provided the full
region that contained the address.

> >  This is not particularly useful and cost me most
> > of a day of debugging.  It also has some integer overflow problems,
> > though as the data comes from dom0 or the firmware (both of which are
> > trusted) these are not security issues.
> 
> I'm afraid we're trusting the firmware in this regard elsewhere as
> well. So if there was a need to change that, I guess it would need
> changing everywhere, not just here. But we trust the E820 map as
> well, when on non-EFI platforms, so I don't see why we would need
> to change that. In any event such would want to be a separate
> change imo.

That is valid.

> > Fix both of these problems by unconditionally setting the memory region
> > size
> 
> If you were to report a larger ending address, why would you not also
> report a smaller starting address?
> 
> But before you go that route - I don't think we can change the API
> now that it has been in use this way for many years. If a "give me
> the full enclosing range" variant is wanted, it will need to be
> fully separate.

Does anyone use this API?

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

On 25.08.2022 22:36, Demi Marie Obenour wrote:
> On Thu, Aug 25, 2022 at 09:59:56AM +0200, Jan Beulich wrote:
>> On 24.08.2022 23:04, Demi Marie Obenour wrote:
>>> Fix both of these problems by unconditionally setting the memory region
>>> size
>>
>> If you were to report a larger ending address, why would you not also
>> report a smaller starting address?
>>
>> But before you go that route - I don't think we can change the API
>> now that it has been in use this way for many years. If a "give me
>> the full enclosing range" variant is wanted, it will need to be
>> fully separate.
> 
> Does anyone use this API?

The XenoLinux forward port of ours did, and upstream Linux still wrongly
doesn't. The two functions efi_mem_type() and efi_mem_attributes() still
wrongly fail there when running on Xen.

But how does this matter? Even if we were unaware of any users of the API,
we can't know there are none.

As an aside: Something's odd with your reply. When I opened the window to
write this reply, Marek and the list were put into To: (instead of Cc:)
and you were dropped altogether. I can only guess that this is what
Thunderbird made of the Mail-Followup-To: tag which your mail has.

Jan

On Fri, Aug 26, 2022 at 09:18:50AM +0200, Jan Beulich wrote:
> On 25.08.2022 22:36, Demi Marie Obenour wrote:
> > On Thu, Aug 25, 2022 at 09:59:56AM +0200, Jan Beulich wrote:
> >> On 24.08.2022 23:04, Demi Marie Obenour wrote:
> >>> Fix both of these problems by unconditionally setting the memory region
> >>> size
> >>
> >> If you were to report a larger ending address, why would you not also
> >> report a smaller starting address?
> >>
> >> But before you go that route - I don't think we can change the API
> >> now that it has been in use this way for many years. If a "give me
> >> the full enclosing range" variant is wanted, it will need to be
> >> fully separate.
> > 
> > Does anyone use this API?
> 
> The XenoLinux forward port of ours did, and upstream Linux still wrongly
> doesn't. The two functions efi_mem_type() and efi_mem_attributes() still
> wrongly fail there when running on Xen.
> 
> But how does this matter? Even if we were unaware of any users of the API,
> we can't know there are none.
> 
> As an aside: Something's odd with your reply. When I opened the window to
> write this reply, Marek and the list were put into To: (instead of Cc:)
> and you were dropped altogether. I can only guess that this is what
> Thunderbird made of the Mail-Followup-To: tag which your mail has.

Probably?  Mutt generated the header because I had (incorrectly)
told it that I am subscribed to xen-devel.  Is it best to leave this
header unset?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

On 26.08.2022 20:15, Demi Marie Obenour wrote:
> On Fri, Aug 26, 2022 at 09:18:50AM +0200, Jan Beulich wrote:
>> On 25.08.2022 22:36, Demi Marie Obenour wrote:
>>> On Thu, Aug 25, 2022 at 09:59:56AM +0200, Jan Beulich wrote:
>>>> On 24.08.2022 23:04, Demi Marie Obenour wrote:
>>>>> Fix both of these problems by unconditionally setting the memory region
>>>>> size
>>>>
>>>> If you were to report a larger ending address, why would you not also
>>>> report a smaller starting address?
>>>>
>>>> But before you go that route - I don't think we can change the API
>>>> now that it has been in use this way for many years. If a "give me
>>>> the full enclosing range" variant is wanted, it will need to be
>>>> fully separate.
>>>
>>> Does anyone use this API?
>>
>> The XenoLinux forward port of ours did, and upstream Linux still wrongly
>> doesn't. The two functions efi_mem_type() and efi_mem_attributes() still
>> wrongly fail there when running on Xen.
>>
>> But how does this matter? Even if we were unaware of any users of the API,
>> we can't know there are none.
>>
>> As an aside: Something's odd with your reply. When I opened the window to
>> write this reply, Marek and the list were put into To: (instead of Cc:)
>> and you were dropped altogether. I can only guess that this is what
>> Thunderbird made of the Mail-Followup-To: tag which your mail has.
> 
> Probably?  Mutt generated the header because I had (incorrectly)
> told it that I am subscribed to xen-devel.  Is it best to leave this
> header unset?

Probably, seeing that it results in misguidance of at least on commonly
used (I believe) frontend.

Jan