public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* SetMemorySpaceAttributes with EFI_MEMORY_XP
@ 2017-03-20 10:32 Michael Zimmermann
  2017-03-20 11:04 ` Ard Biesheuvel
  2017-03-20 11:06 ` Laszlo Ersek
  0 siblings, 2 replies; 11+ messages in thread
From: Michael Zimmermann @ 2017-03-20 10:32 UTC (permalink / raw)
  To: edk2-devel-01, Laszlo Ersek, Ard Biesheuvel

Hi,

I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
for another platform.
So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
being enabled?
https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);

Neither the memory that was added by this Dxe nor the one added
automatically by GCD has the EFI_MEMORY_XP capability which causes
SetMemorySpaceAttributes to return EFI_UNSUPPORTED.

Thanks
Michael


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 10:32 SetMemorySpaceAttributes with EFI_MEMORY_XP Michael Zimmermann
@ 2017-03-20 11:04 ` Ard Biesheuvel
  2017-03-20 11:16   ` Michael Zimmermann
  2017-03-20 11:06 ` Laszlo Ersek
  1 sibling, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 11:04 UTC (permalink / raw)
  To: Michael Zimmermann; +Cc: edk2-devel-01, Laszlo Ersek

On 20 March 2017 at 10:32, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
> Hi,
>
> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
> for another platform.
> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
> being enabled?
> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>
> Neither the memory that was added by this Dxe nor the one added
> automatically by GCD has the EFI_MEMORY_XP capability which causes
> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.
>

That is a very good point. I have been caught by this more than once
already (and I did test this, but not as thoroughly as I should have,
apparently)

This is caused by the unfortunate situation in EDK2 that GCD
permission attributes are ambiguous: it does not distinguish between
'the memory controller allows this range to be configured as
non-executable' and 'the nature of the contents of this memory region
allows it to be mapped without executable attributes', and therefore,
RO/XP are never used in the GCD memory space map.

The solution is to use the CPU_ARCH_PROTOCOL interface explicitly to
set the XP attribute on the memory itself (but not on the descriptors
in the GCD or UEFI memory maps). I will spin a patch to fix this.

Thanks,
Ard.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 10:32 SetMemorySpaceAttributes with EFI_MEMORY_XP Michael Zimmermann
  2017-03-20 11:04 ` Ard Biesheuvel
@ 2017-03-20 11:06 ` Laszlo Ersek
  2017-03-20 11:10   ` Michael Zimmermann
  1 sibling, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-03-20 11:06 UTC (permalink / raw)
  To: Michael Zimmermann, edk2-devel-01, Ard Biesheuvel

On 03/20/17 11:32, Michael Zimmermann wrote:
> Hi,
> 
> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
> for another platform.
> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
> being enabled?
> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
> 
> Neither the memory that was added by this Dxe nor the one added
> automatically by GCD has the EFI_MEMORY_XP capability which causes
> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.

See commit 413edd470932 ("ArmVirtPkg/HighMemDxe: preserve non-exec
permissions on newly added regions", 2017-02-28). EFI_MEMORY_XP is only
requested if EfiConventionalMemory's bit is set in
PcdDxeNxMemoryProtectionPolicy, but then (I think) the DXE_CORE must
have set the same bit in the "supported" bitmask too, when the memory
was added.

Thanks
Laszlo



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 11:06 ` Laszlo Ersek
@ 2017-03-20 11:10   ` Michael Zimmermann
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Zimmermann @ 2017-03-20 11:10 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: edk2-devel-01, Ard Biesheuvel

> but then (I think) the DXE_CORE must
have set the same bit in the "supported" bitmask too, when the memory
was added.
that's the point, if my port behaves the same as ArmVirt, it doesn't.
for all memory, capabilities are 8000000000000008 and attributes are
0000000000000000 for unused memory and 0000000000000008 for used
memory.
This seems to confirm what Ard said about the GCD not being altered
for NX permissions.

Thanks
Michael

On Mon, Mar 20, 2017 at 12:06 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/20/17 11:32, Michael Zimmermann wrote:
>> Hi,
>>
>> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
>> for another platform.
>> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
>> being enabled?
>> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
>> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>>
>> Neither the memory that was added by this Dxe nor the one added
>> automatically by GCD has the EFI_MEMORY_XP capability which causes
>> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.
>
> See commit 413edd470932 ("ArmVirtPkg/HighMemDxe: preserve non-exec
> permissions on newly added regions", 2017-02-28). EFI_MEMORY_XP is only
> requested if EfiConventionalMemory's bit is set in
> PcdDxeNxMemoryProtectionPolicy, but then (I think) the DXE_CORE must
> have set the same bit in the "supported" bitmask too, when the memory
> was added.
>
> Thanks
> Laszlo
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 11:04 ` Ard Biesheuvel
@ 2017-03-20 11:16   ` Michael Zimmermann
  2017-03-20 11:20     ` Ard Biesheuvel
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Zimmermann @ 2017-03-20 11:16 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel-01, Laszlo Ersek

Ard, why is SetMSetMemorySpaceAttributes being called in first place?
(ignoring the recent NX patch)
Looking at the initial GCD, it looks like unused memory usually
doesn't have any attributes set anyway.

Thanks
Michael

On Mon, Mar 20, 2017 at 12:04 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 20 March 2017 at 10:32, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>> Hi,
>>
>> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
>> for another platform.
>> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
>> being enabled?
>> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
>> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>>
>> Neither the memory that was added by this Dxe nor the one added
>> automatically by GCD has the EFI_MEMORY_XP capability which causes
>> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.
>>
>
> That is a very good point. I have been caught by this more than once
> already (and I did test this, but not as thoroughly as I should have,
> apparently)
>
> This is caused by the unfortunate situation in EDK2 that GCD
> permission attributes are ambiguous: it does not distinguish between
> 'the memory controller allows this range to be configured as
> non-executable' and 'the nature of the contents of this memory region
> allows it to be mapped without executable attributes', and therefore,
> RO/XP are never used in the GCD memory space map.
>
> The solution is to use the CPU_ARCH_PROTOCOL interface explicitly to
> set the XP attribute on the memory itself (but not on the descriptors
> in the GCD or UEFI memory maps). I will spin a patch to fix this.
>
> Thanks,
> Ard.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 11:16   ` Michael Zimmermann
@ 2017-03-20 11:20     ` Ard Biesheuvel
  2017-03-20 11:38       ` Laszlo Ersek
  0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 11:20 UTC (permalink / raw)
  To: Michael Zimmermann; +Cc: edk2-devel-01, Laszlo Ersek

On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
> (ignoring the recent NX patch)
> Looking at the initial GCD, it looks like unused memory usually
> doesn't have any attributes set anyway.
>

Originally, we added the new memory with
EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
and selected the EFI_MEMORY_WB attribute with the call to
gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
expect EFI_MEMORY_WB, since the other ones cannot be supported under
virtualization with KVM.

Whether that makes the SetMemorySpaceAttributes redundant is not
entirely clear to me, but it does appear the adding the memory does
the right thing wrt non-exec permissions if the policy is enabled. So
perhaps we can simply drop this call?


> On Mon, Mar 20, 2017 at 12:04 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 20 March 2017 at 10:32, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>>> Hi,
>>>
>>> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
>>> for another platform.
>>> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
>>> being enabled?
>>> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
>>> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>>>
>>> Neither the memory that was added by this Dxe nor the one added
>>> automatically by GCD has the EFI_MEMORY_XP capability which causes
>>> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.
>>>
>>
>> That is a very good point. I have been caught by this more than once
>> already (and I did test this, but not as thoroughly as I should have,
>> apparently)
>>
>> This is caused by the unfortunate situation in EDK2 that GCD
>> permission attributes are ambiguous: it does not distinguish between
>> 'the memory controller allows this range to be configured as
>> non-executable' and 'the nature of the contents of this memory region
>> allows it to be mapped without executable attributes', and therefore,
>> RO/XP are never used in the GCD memory space map.
>>
>> The solution is to use the CPU_ARCH_PROTOCOL interface explicitly to
>> set the XP attribute on the memory itself (but not on the descriptors
>> in the GCD or UEFI memory maps). I will spin a patch to fix this.
>>
>> Thanks,
>> Ard.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 11:20     ` Ard Biesheuvel
@ 2017-03-20 11:38       ` Laszlo Ersek
  2017-03-20 14:08         ` Ard Biesheuvel
  0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-03-20 11:38 UTC (permalink / raw)
  To: Ard Biesheuvel, Michael Zimmermann; +Cc: edk2-devel-01

On 03/20/17 12:20, Ard Biesheuvel wrote:
> On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
>> (ignoring the recent NX patch)
>> Looking at the initial GCD, it looks like unused memory usually
>> doesn't have any attributes set anyway.
>>
> 
> Originally, we added the new memory with
> EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
> and selected the EFI_MEMORY_WB attribute with the call to
> gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
> expect EFI_MEMORY_WB, since the other ones cannot be supported under
> virtualization with KVM.
> 
> Whether that makes the SetMemorySpaceAttributes redundant is not
> entirely clear to me, but it does appear the adding the memory does
> the right thing wrt non-exec permissions if the policy is enabled. So
> perhaps we can simply drop this call?

Won't that turn off caching for the memory just added?

Thanks
Laszlo

> 
> 
>> On Mon, Mar 20, 2017 at 12:04 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 20 March 2017 at 10:32, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
>>>> for another platform.
>>>> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
>>>> being enabled?
>>>> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
>>>> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>>>>
>>>> Neither the memory that was added by this Dxe nor the one added
>>>> automatically by GCD has the EFI_MEMORY_XP capability which causes
>>>> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.
>>>>
>>>
>>> That is a very good point. I have been caught by this more than once
>>> already (and I did test this, but not as thoroughly as I should have,
>>> apparently)
>>>
>>> This is caused by the unfortunate situation in EDK2 that GCD
>>> permission attributes are ambiguous: it does not distinguish between
>>> 'the memory controller allows this range to be configured as
>>> non-executable' and 'the nature of the contents of this memory region
>>> allows it to be mapped without executable attributes', and therefore,
>>> RO/XP are never used in the GCD memory space map.
>>>
>>> The solution is to use the CPU_ARCH_PROTOCOL interface explicitly to
>>> set the XP attribute on the memory itself (but not on the descriptors
>>> in the GCD or UEFI memory maps). I will spin a patch to fix this.
>>>
>>> Thanks,
>>> Ard.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 11:38       ` Laszlo Ersek
@ 2017-03-20 14:08         ` Ard Biesheuvel
  2017-03-20 15:22           ` Yao, Jiewen
  2017-03-20 15:24           ` Laszlo Ersek
  0 siblings, 2 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 14:08 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Michael Zimmermann, edk2-devel-01

On 20 March 2017 at 11:38, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/20/17 12:20, Ard Biesheuvel wrote:
>> On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>>> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
>>> (ignoring the recent NX patch)
>>> Looking at the initial GCD, it looks like unused memory usually
>>> doesn't have any attributes set anyway.
>>>
>>
>> Originally, we added the new memory with
>> EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
>> and selected the EFI_MEMORY_WB attribute with the call to
>> gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
>> expect EFI_MEMORY_WB, since the other ones cannot be supported under
>> virtualization with KVM.
>>
>> Whether that makes the SetMemorySpaceAttributes redundant is not
>> entirely clear to me, but it does appear the adding the memory does
>> the right thing wrt non-exec permissions if the policy is enabled. So
>> perhaps we can simply drop this call?
>
> Won't that turn off caching for the memory just added?
>

I think it may not map the memory at all in this case, so we need to
do something. It looks like calling mCpu->SetMemoryAttributes() should
be sufficient here, and so I wonder whether we violate anything by
replacing gDS->SetMemorySpaceAttributes with mCpu->SetMemoryAttributes
here.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 14:08         ` Ard Biesheuvel
@ 2017-03-20 15:22           ` Yao, Jiewen
  2017-03-20 15:24           ` Laszlo Ersek
  1 sibling, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2017-03-20 15:22 UTC (permalink / raw)
  To: Ard Biesheuvel, Laszlo Ersek; +Cc: edk2-devel-01, Michael Zimmermann

We had some internal discussion on using gDS or using mCpu.

We decided to choose mCpu purposely at that moment, because we wanted to keep UEFI memory map unchanged to avoid any potential compatibility issue.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, March 20, 2017 10:09 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Michael Zimmermann <sigmaepsilon92@gmail.com>
Subject: Re: [edk2] SetMemorySpaceAttributes with EFI_MEMORY_XP

On 20 March 2017 at 11:38, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
> On 03/20/17 12:20, Ard Biesheuvel wrote:
>> On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>> wrote:
>>> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
>>> (ignoring the recent NX patch)
>>> Looking at the initial GCD, it looks like unused memory usually
>>> doesn't have any attributes set anyway.
>>>
>>
>> Originally, we added the new memory with
>> EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
>> and selected the EFI_MEMORY_WB attribute with the call to
>> gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
>> expect EFI_MEMORY_WB, since the other ones cannot be supported under
>> virtualization with KVM.
>>
>> Whether that makes the SetMemorySpaceAttributes redundant is not
>> entirely clear to me, but it does appear the adding the memory does
>> the right thing wrt non-exec permissions if the policy is enabled. So
>> perhaps we can simply drop this call?
>
> Won't that turn off caching for the memory just added?
>

I think it may not map the memory at all in this case, so we need to
do something. It looks like calling mCpu->SetMemoryAttributes() should
be sufficient here, and so I wonder whether we violate anything by
replacing gDS->SetMemorySpaceAttributes with mCpu->SetMemoryAttributes
here.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 14:08         ` Ard Biesheuvel
  2017-03-20 15:22           ` Yao, Jiewen
@ 2017-03-20 15:24           ` Laszlo Ersek
  2017-03-20 19:31             ` Ard Biesheuvel
  1 sibling, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-03-20 15:24 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Michael Zimmermann, edk2-devel-01

On 03/20/17 15:08, Ard Biesheuvel wrote:
> On 20 March 2017 at 11:38, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/20/17 12:20, Ard Biesheuvel wrote:
>>> On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>>>> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
>>>> (ignoring the recent NX patch)
>>>> Looking at the initial GCD, it looks like unused memory usually
>>>> doesn't have any attributes set anyway.
>>>>
>>>
>>> Originally, we added the new memory with
>>> EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
>>> and selected the EFI_MEMORY_WB attribute with the call to
>>> gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
>>> expect EFI_MEMORY_WB, since the other ones cannot be supported under
>>> virtualization with KVM.
>>>
>>> Whether that makes the SetMemorySpaceAttributes redundant is not
>>> entirely clear to me, but it does appear the adding the memory does
>>> the right thing wrt non-exec permissions if the policy is enabled. So
>>> perhaps we can simply drop this call?
>>
>> Won't that turn off caching for the memory just added?
>>
> 
> I think it may not map the memory at all in this case, so we need to
> do something. It looks like calling mCpu->SetMemoryAttributes() should
> be sufficient here, and so I wonder whether we violate anything by
> replacing gDS->SetMemorySpaceAttributes with mCpu->SetMemoryAttributes
> here.

Earlier you mentioned that we cannot get some piece of information from
the GCD map, which limits what we can do here.

Looking at GetMemorySpaceDescriptor() and GetMemorySpaceMap(), they
should return both Capabilities and Attributes.

Also, looking at vol2 in PI 1.5, I find:

* GetMemorySpaceDescriptor() returns EFI_NOT_AVAILABLE_YET if "The
attributes cannot be set because CPU architectural protocol is not
available yet."

* 9.7.3.2 SetMemorySpaceAttributes() -- When the DXE Foundation is
notified that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE
Service SetMemorySpaceAttributes() can be made available. The DXE
Foundation can then use the SetMemoryAttributes() service of the
EFI_CPU_ARCH_PROTOCOL to implement the DXE Service
SetMemorySpaceAttributes().

* 9.7.3.3 GetMemorySpaceMap() -- When the DXE Foundation is notified
that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE Service
GetMemorySpaceMap() is fully functional. This function is made available
when the memory-based services are initialized. However, the Attributes
field of the array of EFI_GCD_MEMORY_SPACE_DESCRIPTORs is not valid
until the EFI_CPU_ARCH_PROTOCOL is installed.

So, assuming that you have tested GetMemorySpaceMap() earlier, and have
found Attributes useless for the protection (or other) purposes, may
that have happened because the CPU arch protocol was not available yet?

... I guess my email is a bit confusing. My points are, (a) we should
likely not mess directly with the CPU arch protocol if we know (and we
do know) that the GCD services use them internally, (b) are we
absolutely sure that the GCD services can't help us here?

If nothing else works, I agree we can mess with the CPU arch protocol
directly.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: SetMemorySpaceAttributes with EFI_MEMORY_XP
  2017-03-20 15:24           ` Laszlo Ersek
@ 2017-03-20 19:31             ` Ard Biesheuvel
  0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 19:31 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Michael Zimmermann, edk2-devel-01

On 20 March 2017 at 15:24, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/20/17 15:08, Ard Biesheuvel wrote:
>> On 20 March 2017 at 11:38, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 03/20/17 12:20, Ard Biesheuvel wrote:
>>>> On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>>>>> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
>>>>> (ignoring the recent NX patch)
>>>>> Looking at the initial GCD, it looks like unused memory usually
>>>>> doesn't have any attributes set anyway.
>>>>>
>>>>
>>>> Originally, we added the new memory with
>>>> EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
>>>> and selected the EFI_MEMORY_WB attribute with the call to
>>>> gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
>>>> expect EFI_MEMORY_WB, since the other ones cannot be supported under
>>>> virtualization with KVM.
>>>>
>>>> Whether that makes the SetMemorySpaceAttributes redundant is not
>>>> entirely clear to me, but it does appear the adding the memory does
>>>> the right thing wrt non-exec permissions if the policy is enabled. So
>>>> perhaps we can simply drop this call?
>>>
>>> Won't that turn off caching for the memory just added?
>>>
>>
>> I think it may not map the memory at all in this case, so we need to
>> do something. It looks like calling mCpu->SetMemoryAttributes() should
>> be sufficient here, and so I wonder whether we violate anything by
>> replacing gDS->SetMemorySpaceAttributes with mCpu->SetMemoryAttributes
>> here.
>
> Earlier you mentioned that we cannot get some piece of information from
> the GCD map, which limits what we can do here.
>
> Looking at GetMemorySpaceDescriptor() and GetMemorySpaceMap(), they
> should return both Capabilities and Attributes.
>
> Also, looking at vol2 in PI 1.5, I find:
>
> * GetMemorySpaceDescriptor() returns EFI_NOT_AVAILABLE_YET if "The
> attributes cannot be set because CPU architectural protocol is not
> available yet."
>
> * 9.7.3.2 SetMemorySpaceAttributes() -- When the DXE Foundation is
> notified that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE
> Service SetMemorySpaceAttributes() can be made available. The DXE
> Foundation can then use the SetMemoryAttributes() service of the
> EFI_CPU_ARCH_PROTOCOL to implement the DXE Service
> SetMemorySpaceAttributes().
>
> * 9.7.3.3 GetMemorySpaceMap() -- When the DXE Foundation is notified
> that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE Service
> GetMemorySpaceMap() is fully functional. This function is made available
> when the memory-based services are initialized. However, the Attributes
> field of the array of EFI_GCD_MEMORY_SPACE_DESCRIPTORs is not valid
> until the EFI_CPU_ARCH_PROTOCOL is installed.
>

OK, that's the theory. The reality (as Jiewen confirms) is that the
RO/XP capability and attribute bits are never used on the GCD side,
because they may cause compatibility issues.

The problem is that UEFI memory map entries inherit all attributes
from the GCD memory space entries they are carved out of. This means
that all UEFI memory map entries will have RO and XP set, which is
clearly not what we want.

I guess it would be feasible to filter out RO and XP when calling
CoreAddRange(), but that does not happen currently.

> So, assuming that you have tested GetMemorySpaceMap() earlier, and have
> found Attributes useless for the protection (or other) purposes, may
> that have happened because the CPU arch protocol was not available yet?
>

No

> ... I guess my email is a bit confusing. My points are, (a) we should
> likely not mess directly with the CPU arch protocol if we know (and we
> do know) that the GCD services use them internally, (b) are we
> absolutely sure that the GCD services can't help us here?
>
> If nothing else works, I agree we can mess with the CPU arch protocol
> directly.
>

I'm afraid that is our only option.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-03-20 19:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-20 10:32 SetMemorySpaceAttributes with EFI_MEMORY_XP Michael Zimmermann
2017-03-20 11:04 ` Ard Biesheuvel
2017-03-20 11:16   ` Michael Zimmermann
2017-03-20 11:20     ` Ard Biesheuvel
2017-03-20 11:38       ` Laszlo Ersek
2017-03-20 14:08         ` Ard Biesheuvel
2017-03-20 15:22           ` Yao, Jiewen
2017-03-20 15:24           ` Laszlo Ersek
2017-03-20 19:31             ` Ard Biesheuvel
2017-03-20 11:06 ` Laszlo Ersek
2017-03-20 11:10   ` Michael Zimmermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox