Message ID | 20250421162712.77452-1-ross.philipson@oracle.com |
---|---|
Headers | show |
Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
On 21/04/2025 9:52 pm, Dave Hansen wrote: > On 4/21/25 09:26, Ross Philipson wrote: >> The larger focus of the TrenchBoot project (https://github.com/TrenchBoot) is to >> enhance the boot security and integrity in a unified manner. > Hey Folks, > > It isn't immediately apparent what these 5,000 lines of code do which is > new, why they are important to users and who will use them. I've > wondered this from v1 and I was hoping it would have gotten better by > v14, but alas... > > Purely from the amount of interest and review tags and the whole "v14" > thing, it doesn't look like this is very important to anyone. Not to be > to flippant about it, but if nobody else cares, why should I (or the > other x86 maintainers)? The very-tl;dr is: This is an implementation of Intel TXT which isn't a piece of abandonware with unaddressed CVEs (i.e. isn't tboot). AMD and ARM support of equivalent technologies will be coming next. ~Andrew
On 4/21/25 09:27, Ross Philipson wrote: > +static u64 sl_rdmsr(u32 reg) > +{ > + u64 lo, hi; > + > + asm volatile ("rdmsr" : "=a" (lo), "=d" (hi) : "c" (reg)); > + > + return (hi << 32) | lo; > +} Is there a reason this code doesn't just use boot_rdmsr()?
On 21/04/2025 9:52 pm, Dave Hansen wrote: > Purely from the amount of interest and review tags and the whole "v14" > thing, it doesn't look like this is very important to anyone. Not to be > to flippant about it, but if nobody else cares, why should I (or the > other x86 maintainers)? There are several downstreams already using this as a part of their overall system security, one example being https://www.qubes-os.org/doc/anti-evil-maid/ It's all giant out-of-tree patch series (in multiple projects; Grub, Xen, iPXE too). Ross and others are trying to be good open source citizen and put it upstream where yet-more downstreams can benefit too. ~Andrew
On 4/21/25 3:57 PM, Dave Hansen wrote: > On 4/21/25 09:27, Ross Philipson wrote: >> @@ -788,6 +790,9 @@ static void native_machine_halt(void) >> >> tboot_shutdown(TB_SHUTDOWN_HALT); >> >> + /* SEXIT done after machine_shutdown() to meet TXT requirements */ >> + slaunch_finalize(1); > > This is the kind of stuff that needs to get fixed up before this series > can go _anywhere_. > > "TXT requirements" is not useful to a maintainer. *WHAT* requirement? > *WHY* must it be done this way? > > This code is unmaintainable as it stands. Sorry we understand the frustration especially for maintainers. We have gone over your responses so far. We will do whatever it takes to make this patch set maintainable and acceptable to upstream. I think we are starting to understand what the main issues are with the set overall from what you are pointing out. Thank you for your feedback, Ross
On 4/22/25 14:26, Ard Biesheuvel wrote: > So if that is true (I'm not a x86 uarch expert by any measure), then > pushing back on this series on the basis that it is ugly and intrusive > is not really reasonable. From security pov, I think D-RTM is an > important feature and it deserves to be upstream if it is used widely > in the field. BTW, I'm not pushing back on it for being intrusive. It's actually not _that_ intrusive. Most of it sits off on the side. It looked like it had a parallel boot entry point, for instance, that is separate from but shouldn't break the normal entry points. BTW. it sounds like folks are pretty unhappy with Intel here on a bunch of levels. It's not my personal hardware design or anything, so please don't pull any punches. If Intel screwed up here and that's why we need 5,000 lines of arguably duplicate functionality, then please say so. You're not going to hurt my feelings.