From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4BD9721130733 for ; Fri, 14 Sep 2018 02:27:07 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E238A3091749; Fri, 14 Sep 2018 09:27:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-199.rdu2.redhat.com [10.10.120.199]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3296DA85AF; Fri, 14 Sep 2018 09:27:05 +0000 (UTC) To: "Wang, Jian J" , Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , "Zeng, Star" , "Ni, Ruiyu" , "Yao, Jiewen" References: <20180914051335.2644-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <2473b451-8d78-cf3f-9cff-80e70de91007@redhat.com> Date: Fri, 14 Sep 2018 11:27:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Fri, 14 Sep 2018 09:27:07 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Sep 2018 09:27:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 < 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