On 06/29/16 15:02, Yao, Jiewen wrote:
> HI Laszlo
>
> I appreciate your careful review.
>
> I would like to clarify why we did not consider adding check for integer
> overflow.
>
> The EDKII security model is that:
>
> 1) We trust any internal data communication before EndOfDxe. We
> have to, because the SMM core has to trust the component runs before
> SMM, and construct SMM.
>
> 2) We do not trust any external data communication after EndOfDxe.
> We should not, external data might be from an untrusted source. That is
> why we need lots of check in PE/COFF library, SMM communication buffer,
> UEFI authenticated variable, etc.
Ah, thanks. This makes a lot of sense. I'll try to remember this.
It's just that I'm so much aware of integer overflows that I routinely
formulate all my arithmetics and comparisons with them in mind,
regardless of the origin of the data :)
> Based upon above assumption, SmramRanges is from a trusted source. There
> is **no need** to add security check, we just need add function check.
Understood.
[snip]
> Go back to your suggestion. I have no problem to adopt you suggestion on
> using below format. It is not harmful.
>
> if (PhysicalSize <= MAX_ADDRESS &&
> CpuStart - 1 <= MAX_ADDRESS - PhysicalSize) {
>
>
>
> But one thing I have to mention is that: If we do not trust CpuStart and
> PhysicalSize, we will have big problem, because CPU driver need use
> these data to set SMRR, which is the basic of SMM security.
>
> We have assume CpuStart and PhysicalSize is set correctly.
In that case, I think the awkward rearrangement is not necessary, and
should be avoided for clarity's sake.
However, the "minus one" does remain necessary, because the (pre-patch)
4GB limit is an exclusive one, while the (post-patch) MAX_ADDRESS limit
is an inclusive one. So my point (2) remains valid, in my opinion:
CpuStart + PhysicalSize - 1 <= MAX_ADDRESS
Do you agree?
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
