Re: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs

[Laszlo Ersek <lersek@redhat.com>]

On 09/14/18 08:50, Wang, Jian J wrote:
> Ard,
>
>> Why? Is it unreasonable to protect the stack but not other
>> EfiBootServicesData regions?
>
> I think you got me wrong. It's reasonable to protect stack but not
> other EfiBootServicesData regions. What I want to express in the
> commit message is the contrary one is not: not to protect stack but
> protect all EfiBootServicesData regions. I have one statement at the
> end of message saying "PcdSetNxForStack and PcdImageProtectionPolicy
> have priority over PcdDxeNxMemoryProtectionPolicy". Do you agree with
> it?
>
> The examples I gave are unreasonable ones. I can't image any
> reason to try those combinations on purpose. But if there's really
> good reason to do it, it's ok to remove those checks.

The issue is with the wording. "Taking priority" means "resolving an
inconsistency by force". That is, we have two settings that contradict
each other, but one of those settings is considered stronger, and the
other weaker. Thus the former takes priority, and the latter is ignored.

But that's not what we're doing here. We're ensuring that there is no
inconsistency in the first place. If there is, we catch it with an
assert. Therefore there is no priority ordering between the settings,
they are equally strong, and they must not be in disagreement.

(PcdSetNxForStack being set, and PcdDxeNxMemoryProtectionPolicy being
clear, are *not* inconsistent; so there is no need to give priority to
one over the other.)

The word that we are looking for is "implies". In logic, if we have a
formula

  A --> B

we read it as "A implies B". The expression is true if "A" is false
(everything follows from false, or put differently, false implies
everything), or else, if "A" is true, then "B" must be true as well.
Otherwise the expression evaluates to true.

In the C language, we don't have an "imply" operator. Luckily, the same
predicate (the implication) can be written with the logical negation and
"or" operators, like this:

  !A || B

It is exactly the same thing. If "A" is false, then the expression is
true (regardless of the value of "B"). If "A" is true, then the
expression is only true if "B" is true as well (that is, if "A" in fact
implies "B").


So, back to the concrete topic. The original predicate we want to ensure
is the following [1]:

  ((DxeNxMemoryProtectionPolicy covers BSD) --> SetNxForStack) AND
  ((DxeNxMemoryProtectionPolicy covers RSD) --> ImageProtectionPolicy) AND
  ((DxeNxMemoryProtectionPolicy covers BSD) --> ImageProtectionPolicy) AND
  ((DxeNxMemoryProtectionPolicy covers LD) --> ImageProtectionPolicy)

In English language, this reads:

- if DxeNxMemoryProtectionPolicy covers BootServicesData, then
  SetNxForStack must be true (the former "implies" the latter), AND

- if DxeNxMemoryProtectionPolicy covers RuntimeServicesData, then
  PcdImageProtectionPolicy must be nonzero (the former "implies" the
  latter), AND

- ... so on and so forth.

(We are past *why* we want to ensure these, I think, but I can repeat
it, for the first one anyway: the stack is mostly located in boot
services data type memory, and marking all that memory NX via
DxeNxMemoryProtectionPolicy, while *not* marking the stack subset of it
via SetNxForStack, would be counter-intuitive / self-contradictory.

When both of those settings are "on", that's fine.

It's also fine when *only* SetNxForStack is "on".

This means that the statement "DxeNxMemoryProtectionPolicy covers BSD"
*implies* (or "requires") that SetNxForStack is "on".)


Now, in order to eliminate the "implies" operator, we can transcribe the
predicate as follows, using the equivalence I mentioned at the top
(i.e., (A --> B) === (!A || B)):

  (NOT(DxeNxMemoryProtectionPolicy covers BSD) OR SetNxForStack) AND
  (NOT(DxeNxMemoryProtectionPolicy covers RSD) OR ImageProtectionPolicy) AND
  (NOT(DxeNxMemoryProtectionPolicy covers BSD) OR ImageProtectionPolicy) AND
  (NOT(DxeNxMemoryProtectionPolicy covers LD) OR ImageProtectionPolicy)

Finally, in order to calculate the condition for triggering the
*failure* case, we have to negate the above predicate. It starts with
slapping a huge NOT() around the whole predicate, of course; but we can
do better, by pushing down NOT(). And for that, we use De Morgan's Laws,
that is:

  !(A || B) === !A && !B
  !(A && B) === !A || !B

which, after repeated application, gives us for the failure condition:

  ((DxeNxMemoryProtectionPolicy covers BSD) AND NOT(SetNxForStack)) OR
  ((DxeNxMemoryProtectionPolicy covers RSD) AND NOT(ImageProtectionPolicy)) OR
  ((DxeNxMemoryProtectionPolicy covers BSD) AND NOT(ImageProtectionPolicy)) OR
  ((DxeNxMemoryProtectionPolicy covers LD) AND NOT(ImageProtectionPolicy))

This is exactly what Jian wrote in the commit message (not accounting
for the short-circuit behavior of the && operator -- but in this case,
short-circuiting doesn't matter, because there are no side effects to
the sub-expressions):

    PcdSetNxForStack == FALSE &&
      (PcdDxeNxMemoryProtectionPolicy & (1 <<EfiBootServicesData)) != 0

    PcdImageProtectionPolicy == 0 &&
      (PcdDxeNxMemoryProtectionPolicy & (1 << EfiRuntimeServicesData)) != 0

    PcdImageProtectionPolicy == 0 &&
      (PcdDxeNxMemoryProtectionPolicy & (1 <<EfiBootServicesData)) != 0

    PcdImageProtectionPolicy == 0 &&
      (PcdDxeNxMemoryProtectionPolicy & (1 <<EfiLoaderData)) != 0

I believe we agree on the desired condition, it's just that the commit
message should not say

> PcdSetNxForStack and PcdImageProtectionPolicy have priority over
> PcdDxeNxMemoryProtectionPolicy

instead it should cite our consistency requirement (see [1] above).

--*--

Actually, I do have a suggestion for improving the C-language condition
further (the logical predicate is fine as-is). I suggest replacing

  PcdImageProtectionPolicy == 0

with

  PcdImageProtectionPolicy != 3

Because, PcdDxeNxMemoryProtectionPolicy will mark the data section
non-executable *regardless* of where any given image comes from (unknown
device or firmware volume). Therefore, if PcdDxeNxMemoryProtectionPolicy
is "on" (for RSD / BSD / LD), then the platform must explicitly request
image protection for images from *both* origins.

Thanks
Laszlo