Re: [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

On 6/1/2023 11:11 AM, Ard Biesheuvel wrote:
> On Thu, 1 Jun 2023 at 16:53, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> Thanks for sending out this RFC, great to see more work on the memory
>> protections front. A few questions and thoughts:
>>
>> This seems a good effort (in conjunction with your last RFC) to close
>> the protection gap between DxeCore launch and CpuDxe launch for marking
>> non-code regions NX. Do you see other protections (guard pages for
>> example) fitting into this method? I believe for any dynamic protections
>> during this timeframe we would need the ability to manipulate the page
>> tables directly from DxeCore.
>>
> 
> The use case of guard pages did not really occur to me, to be honest,
> and this is obviously something that doesn't work either before the
> CPU arch protocol is dispatched
> 
> I still think it would be preferable to add the ability to manage
> memory mapping permissions to the DXE core itself, and separate it
> from the CPU arch protocol.
> 
> Note that clumping everything together does not really help in this
> respect either: if the memory permission manipulation logically
> remains a part of the CPU arch protocol, which cannot be installed
> until its dependencies are satisfied, we are still in a situation
> where dispatching those dependencies may result in page allocations
> being created before we can unmap the guard pages.
> 

I agree. I think that the ability for DXE Core to manage the memory
attributes itself is the central simplification that could go a long
ways towards cleaning up the existing interfaces and making the code
more maintainable with less potential pitfalls (that we currently see
hit frequently).

I think the memory attribute management can be split apart from CpuDxe
without bringing all of CpuDxe into Dxe Core and certainly if that is
the better path towards tackling the problem we face, I support it.

>> Similarly, in order to lessen the complexity of the DXE driver usage of
>> memory resources and avoiding sync issues (e.g. a driver allocates pages
>> that are mapped NX by default, then it sets a cacheability attribute
>> and accidentally clears NX), I think further work would be valuable to
>> reduce that complexity. I think your new PPI that allows setting and
>> preserving bits independently of what is passed in is a very good step
>> towards reducing this complexity.
>>
> 
> Hopefully, we'll be able to do something at the library/driver level
> here (AllocatePages in the DMA or PCI layer).
> 
> Another thing we might entertain (which maps really well onto the WXN
> thing we have on ARM) is to add a GCD memory region capability that
> makes memory XP unless it is RO. But I haven't really experimented
> with that yet - I'll keep you posted on that.
> 

This is definitely interesting to me as well. I would love this
capability :). I've been doing some investigations in this area,
but not specifically towards ARM's WXN, that seems a great place
to intersect.

Thanks,
Oliver

>> This patchset would move all properly aligned DXE drivers to be XIP,
>> correct?
> 
> Yes.
> 
>> Because we are XIP in DRAM, this should not have any
>> performance implications (other than a benefit from reducing the extra
>> copies in your first few patches), aside from potential space
>> differences, which as you note compression will likely do away with,
>> right?
>>
> 
> Exactly.
> 
> 
> 
>