The SEV-ES stacks may share page with code and other data. This does not only risk the other data to be overwritten by stack overflows, it also prevents stack guarding and NX stack. [1] https://github.com/tianocore/edk2/blob/2072c22a0d63c780b0cc6377f6d4ffb116ad6144/UefiCpuPkg/Library/MpInitLib/MpLib.c#L1171-L1179 [2] https://github.com/tianocore/edk2/blob/2072c22a0d63c780b0cc6377f6d4ffb116ad6144/UefiCpuPkg/Library/MpInitLib/MpLib.c#L1212
Thomas Lendacky, I added you to CC list, to bring this BZ to your attention. Please review.
Would a separate allocation, that is page aligned/page sized, resolve the concern? In other words, for example during PEI: - Have the reset vector code and data be at 9F000 for 0x1000 - Have the SEV-ES AP reset stacks be at 9E000 for 0x1000 (variable based on vCPU count) below the reset vector code and data
Thank you for your reply, Thomas. That will definitely improve the situation, but I am wondering whether it would not make sense to go a step further and separate the individual AP stacks, so that guarding pages can be placed on each's top and bottom. To not waste too much space, I guess they can remain contiguous (as in the top guard page of one stack doubles as the bottom guard page of another), but that'd need explicit support then (as guard pages are otherwise automatically inserted by DxeCore according to the PCD policy per page allocation). I don't think I can comment on the reset vector situation as I lack information on data content and page protection on entry. I trust your judgement. :)
(In reply to Marvin Häuser from comment #3) > Thank you for your reply, Thomas. That will definitely improve the > situation, but I am wondering whether it would not make sense to go a step > further and separate the individual AP stacks, so that guarding pages can be > placed on each's top and bottom. To not waste too much space, I guess they > can remain contiguous (as in the top guard page of one stack doubles as the > bottom guard page of another), but that'd need explicit support then (as > guard pages are otherwise automatically inserted by DxeCore according to the > PCD policy per page allocation). IIUC, that would be two pages per AP + one initial top page? If that is the case, I don't think we can do that because we have a limited number of pages available below 1MB. For example, when I boot a VM today, the current range below 1MB in PEI is 0 - 0xA0000, which is only 160 pages. It would impose a much stricter limit on the number of vCPUs that can be specified. > > I don't think I can comment on the reset vector situation as I lack > information on data content and page protection on entry. I trust your > judgement. :)
Oh, the stacks must be below 1 MB too? That's unfortunate. :( Thank you!
v1: https://edk2.groups.io/g/devel/message/75033 https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00349.html msgid: <1162d1f4fe09048aaafba6f6ea3046bebd34fc2b.1620766224.git.thomas.lendacky@amd.com>
Hi Tom, (In reply to thomas.lendacky from comment #6) > v1: > https://edk2.groups.io/g/devel/message/75033 > > https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00349.html > msgid: > <1162d1f4fe09048aaafba6f6ea3046bebd34fc2b.1620766224.git.thomas.lendacky@amd. > com> this BZ has been filed as a Security Issue. Personally, I disagree that this is a security issue (it doesn't seem exploitable). I would rather call it an "armoring feature request". *However*, even though that's my personal opinion, as long as the BZ *is* filed as a (non-public) security issue, we should consider it embargoed, and we should neither discuss it publicly, nor post patches for it to the list. I'd support opening this ticket up to the public, but until / unless that happens, we shouldn't just post patches to the list, before an embargo date is set, and the set date elapses. https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Security-Issues While bugzilla is *by far* not the perfect tool for patch reviews, for the time being we attach security patches to BZ tickets, for review. So all the discussions around the symptoms, impact, CVSS score, embargo date, fixes, reviews etc happen in the ticket, for a security issue. Once the embargo ends, the ticket is opened up and the latest patches from the BZ are reposted to the edk2-devel mailing list, a bit of time is given to the community for a final round of review, and then the patches are merged upstream. Regarding the current patch review -- personally I'd like to defer that to Marvin (primarily) and the UefiCpuPkg maintainers (secondarily).
Hey Laszlo, Sorry, I'm actually not sure why I filed this as security issue. I filed I think 17 tickets that day, so maybe I simply got it mixed up. :) Of course you can decide on whether to open it, but if it is fine for me to do it as the reporter, I absolutely will. Hey Thomas, Thank you a lot for the patch. Sorry, but it will take me a bit to get to it. First glance is good and definitely addresses the issue.
I should also mention that the infosec group has a monthly phone (maybe video?) meeting where they go through the security BZ backlog, and that's when eligible BZs get opened up, based on the live discussion and the prior comments in the tickets. I usually don't dial in to the meeting, but seek to keep a close eye on tickets, and to state quickly (usually, although not always, with the help of Red Hat ProdSec) whether a ticket should be embargoed, and if so, for how long, in our opinion. This one, I don't see as directly exploitable. It may be a gap in a generic protection mechanism, yes. So personally I don't mind that the patch was posted, but it did not conform to the process, as far as I can tell. Best I can offer is some regression testing (in non-SEV guests), once we figure out where the patch should be discussed, going forward. (Unfortunately I don't have a SEV-ES environment set up yet, for regression-testing in a SEV-ES guest.)
(In reply to Marvin Häuser from comment #8) > Hey Laszlo, > > Sorry, I'm actually not sure why I filed this as security issue. I filed I > think 17 tickets that day, so maybe I simply got it mixed up. :) Of course > you can decide on whether to open it, but if it is fine for me to do it as > the reporter, I absolutely will. I think it never hurts to file a BZ as a Security Issue at first if the reporter has any concerns about the potential exploitability of the problem -- people can always discuss on the BZ and then decide to open it up. Looks like you had no such concerns however, when filing this ticket. Tom, can you please comment? Do you agree as well that we shouldn't treat this as a security issue? If that's the case, then I'd propose that Marvin do open up the ticket. Thanks!
(In reply to Laszlo Ersek from comment #10) > > Tom, can you please comment? Do you agree as well that we shouldn't treat > this as a security issue? If that's the case, then I'd propose that Marvin > do open up the ticket. Thanks! Yes, I agree, I don't see this as a security issue. And my bad on posting patches. Since there wasn't a paper or method of exploitation mentioned I didn't think there was any kind of embargo period. For my education, would all patch review for a security issue be done within the bugzilla or is there a special mailing list or ... ?
Hi Tom, Personally, I would strongly prefer to use the existent <infosec@edk2.groups.io> mailing list for security patch review, but many other project participants disagreed, saying that such a mailing list is not confidential enough. So we've been stuck with security patch reviews through bugzilla attachments, for a while. There isn't any more recent guidance that I could cite here. Thanks!
v2: https://edk2.groups.io/g/devel/message/75132 https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00448.html msgid: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com>
Merged in commit dbc22a178546 via <https://github.com/tianocore/edk2/pull/1674>.
