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

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

[Laszlo Ersek]

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

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

[Yao, Jiewen]

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. I believe we need add below check in UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c, ConvertMemoryPageAttributes()

==========================
if (BaseAddress > MAX_ADDRESS) {
  return RETURN_UNSUPPORTED;
}
if (Length > MAX_ADDRESS) {
  return RETURN_UNSUPPORTED;
}
if ((Length != 0) && (BaseAddress > MAX_ADDRESS - (Length - 1))) {
  return RETURN_UNSUPPORTED;
}
==========================
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?

Thank you
Yao Jiewen



> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, July 8, 2017 10:12 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <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

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

[Laszlo Ersek]

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>
>> Cc: edk2-devel-01 <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

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

[Yao, Jiewen]

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