Re: memory protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks Laszlo.

I agree with you that we can support Base = 0, Length = MAX_ADDR+1. The last page is MAX_ADDR+1-PAGE_SIZE, which is a valid value.

I do not think it is necessary to support Base=MAX_ADDR+1, Length=0. It does not match any entry in the page table.

Thank you
Yao Jiewen

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Sunday, July 9, 2017 3:50 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>
Subject: Re: memory protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB

On 07/08/17 15:38, Yao, Jiewen wrote:
> Thanks Laszlo. I think this is a special case we did not test before. And it is a bug we need fix.
>
> Unfortunately, I am out of office these days with limited email access. I just saw the email today.
>
>
> I have a quick look at the code.

Thank you for your help, and I'm sorry about disturbing you while you
are out of office.

> I believe we need add below check in UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c, ConvertMemoryPageAttributes()
>
> ==========================
> if (BaseAddress > MAX_ADDRESS) {
>   return RETURN_UNSUPPORTED;
> }

Mathematically speaking (not in C expressions), I think
BaseAddress:=(MAX_ADDRESS+1) with Length:=0 is valid as well. (In theory
anyway.)

So I would replace this check as follows:

if ((BaseAddress > 0) && ((BaseAddress - 1) > MAX_ADDRESS)) {
  return RETURN_UNSUPPORTED;
}

> if (Length > MAX_ADDRESS) {
>   return RETURN_UNSUPPORTED;
> }

Mathematically speaking (not in C expressions), I think BaseAddress:=0
with Length:=(MAX_ADDRESS+1) is valid too. (In theory anyway.)

So, I would replace this check as follows:

if ((Length > 0) && ((Length - 1) > MAX_ADDRESS)) {
  return RETURN_UNSUPPORTED;
}

> if ((Length != 0) && (BaseAddress > MAX_ADDRESS - (Length - 1))) {
>   return RETURN_UNSUPPORTED;
> }

Yes, this looks good. And, it would work correctly with the above two
modifications as well; it will accept both of the mentioned "corner cases".

(Anyway I haven't looked at the source, and the difference is purely
theoretical.)

> ==========================
> to filter invalid address in IA32.
>
>
> (Well, it is valid for OS, because OS may use PAE to match to lower. But it is invalid for UEFI, because UEFI uses identical address)
>
>
> Would you please file an HSD for that?

What does HSD stand for? :)

Either way, I've filed:

https://bugzilla.tianocore.org/show_bug.cgi?id=624

Thank you!
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Saturday, July 8, 2017 10:12 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
>> Subject: memory protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM
>> above 4GB
>>
>> Hi Jiewen,
>>
>> I just noticed that building OvmfIa32.dsc with -D SMM_REQUIRE, and then
>> running the 32-bit guest with 4G RAM (of which 2GB are placed in 64-bit
>> address pace), the guest crashes when PiSmmCpuDxeSmm tries to protect
>> the memory range at 4GB. Please find the log attached (it ends with the
>> crash).
>>
>> Is this expected to work?
>>
>> Thanks
>> Laszlo