On handling vendorcode

[Patrick Georgi]

Hi everybody,

We recently landed a change (https://review.coreboot.org/51827) to be more
selective which parts of src/vendorcode are checked for coding style
because some areas are really coreboot code "by vendors".

The original purpose of that subhierarchy was to isolate code we draw in
from the outside to make every dev aware that the file they're touching has
some upstream other than coreboot and that this code shouldn't be modified
except to track that upstream.

Any objections to moving the code out there that has no other upstream
(e.g. src/vendorcode/google/chromeos or src/vendorcode/eltan, I think?)
while moving in code from elsewhere in the tree that fits the "it has a
foreign upstream" description (e.g. the lzma library)?

Regards,
Patrick

[Peter Stuge]

Patrick Georgi via coreboot wrote:
> Any objections to moving the code out there that has no other upstream
> (e.g. src/vendorcode/google/chromeos or src/vendorcode/eltan, I think?)
> while moving in code from elsewhere in the tree that fits the "it has a
> foreign upstream" description (e.g. the lzma library)?

I think that can make sense, but where would the chromeos, eltan and
other directories go instead? Someplace existing, or someplace new?

//Peter

[Patrick Georgi]

Am Di., 6. Apr. 2021 um 21:21 Uhr schrieb Peter Stuge :
> Patrick Georgi via coreboot wrote:
> > Any objections to moving the code out there that has no other upstream
> > (e.g. src/vendorcode/google/chromeos or src/vendorcode/eltan, I think?)
> > while moving in code from elsewhere in the tree that fits the "it has a
> > foreign upstream" description (e.g. the lzma library)?
>
> I think that can make sense, but where would the chromeos, eltan and
> other directories go instead? Someplace existing, or someplace new?

I first wanted to make sure that there are no major issues with the overall
idea, but to further the discussion:

- src/vendorcode/eltan/security -> src/security/mboot
- src/vendorcode/google/chromeos is a mixed bag: some src/drivers/tpm/cr50,
some src/lib (e.g. elog, vpd), some src/acpi/chromeos (acpi*)

I'm not sure about Siemens' hwilib: if that's imported, it remains there,
if this version is written for coreboot, src/drivers/siemens perhaps?

The other things look to be "real" vendorcode, even if we're probably the
last codebase by now that still ships (our versions of) amd/agesa.

Patrick

Hi Patrick.
> I'm not sure about Siemens' hwilib: if that's imported, it remains there, if this version is written for coreboot, src/drivers/siemens perhaps?

This code is basically a pure software library to access defined fields within a binary image kept inside CBFS.
Since there is no hardware this code especially deals with I am not sure about src/drivers/ as the host directory.
But I am totally fine with moving it into the tree and out of vendorcode, maybe into src/lib/siemens?

Werner

Von: Patrick Georgi via coreboot  
Gesendet: Mittwoch, 7. April 2021 10:59
An: Peter Stuge 
Cc: coreboot 
Betreff: [coreboot] Re: On handling vendorcode

Am Di., 6. Apr. 2021 um 21:21 Uhr schrieb Peter Stuge :
Patrick Georgi via coreboot wrote:
> Any objections to moving the code out there that has no other upstream
> (e.g. src/vendorcode/google/chromeos or src/vendorcode/eltan, I think?)
> while moving in code from elsewhere in the tree that fits the "it has a
> foreign upstream" description (e.g. the lzma library)?

I think that can make sense, but where would the chromeos, eltan and
other directories go instead? Someplace existing, or someplace new?
I first wanted to make sure that there are no major issues with the overall idea, but to further the discussion:

- src/vendorcode/eltan/security -> src/security/mboot
- src/vendorcode/google/chromeos is a mixed bag: some src/drivers/tpm/cr50, some src/lib (e.g. elog, vpd), some src/acpi/chromeos (acpi*)

I'm not sure about Siemens' hwilib: if that's imported, it remains there, if this version is written for coreboot, src/drivers/siemens perhaps?

The other things look to be "real" vendorcode, even if we're probably the last codebase by now that still ships (our versions of) amd/agesa.

Patrick

[Julius Werner]

> Any objections to moving the code out there that has no other upstream (e.g. src/vendorcode/google/chromeos or src/vendorcode/eltan, I think?) while moving in code from elsewhere in the tree that fits the "it has a foreign upstream" description (e.g. the lzma library)?

Sorry, I just responded to this on
https://review.coreboot.org/c/coreboot/+/51827 before I saw this mail.

I think we still need to have a difference between hacky vendor stuff
and normal coreboot code. For example, the Eltan mboot stuff is
something we didn't really want to have in coreboot in that form, and
so they kinda put it in vendorcode as a compromise. We should make
sure it remains clear that that code isn't "proper" coreboot code and
didn't go through the same level of review.

If you want to separate the two cases, maybe just make a
src/vendorcode/mirrored/ subdirectory and put the stuff that was
mirrored from a different upstream under there?

[Patrick Georgi]

Am Mi., 7. Apr. 2021 um 01:12 Uhr schrieb Julius Werner <
[e-mail]>:
> I think we still need to have a difference between hacky vendor stuff
> and normal coreboot code. For example, the Eltan mboot stuff is
> something we didn't really want to have in coreboot in that form, and
> so they kinda put it in vendorcode as a compromise. We should make
> sure it remains clear that that code isn't "proper" coreboot code and
> didn't go through the same level of review.

It might have started that way, but I don't think that's an accurate
portrayal of eltan's work at this point:
The eltan code uses vboot for the cryptographic primitives these days and
as far as I can see, extends it for measured boot - which vboot itself
doesn't do, ever.

Also, regarding your point on gerrit (collecting arguments in this thread)
that we don't have duplicate things, look no further than graphics init:
- src/device/oprom/realmode
- src/device/oprom/yabel
- src/drivers/intel/gma
- FSP can do the graphics init, too (and report it in cbtables)

(I didn't count the native ARM graphics init routines because we don't ship
alternatives for those)

Patrick

[Julius Werner]

> > I think we still need to have a difference between hacky vendor stuff
> > and normal coreboot code. For example, the Eltan mboot stuff is
> > something we didn't really want to have in coreboot in that form, and
> > so they kinda put it in vendorcode as a compromise. We should make
> > sure it remains clear that that code isn't "proper" coreboot code and
> > didn't go through the same level of review.
>
> It might have started that way, but I don't think that's an accurate portrayal of eltan's work at this point:
> The eltan code uses vboot for the cryptographic primitives these days and as far as I can see, extends it for measured boot - which vboot itself doesn't do, ever.

No, that wasn't the only problem with that code. It is implementing
things that we already have an official solution for and doing that in
ways that aren't really safe and hinder development on core
components. For example, this is the only user of prog_locate_hook(),
a function that doesn't even really make sense anymore because I
recently removed the prog_locate() function it was based on, and that
was always a bad design to begin with because it fundamentally
requires the same data to be loaded more than once to do the stuff
that Eltan's mboot is trying to do with it (which is both inefficient
and an opening for TOCTOU attacks).

We already have a canonical verification solution (CONFIG_VBOOT) and a
canonical measurement solution (CONFIG_TPM_MEASURED_BOOT), and we
should be aiming to make those flexible enough to solve everyone's use
case rather than have everyone implement the same thing slightly
differently. It's much better to have one flexible, well-maintained
solution than a dozen different, poorly-maintained solutions. Those
problems I mentioned about double loading and TOCTOU safety can all be
solved, and I'm aiming to do that with the CBFS verification work I'm
currently doing, but it's complicated and it requires integration into
CBFS internals. I only want to do and maintain that once and not once
for every different vendor implementation. And doing development on
this stuff is a lot easier when you don't need to maintain weird,
badly designed hooks that someone once crammed in there to support a
one-off solution.

Eltan already signaled they were going to deprecate the mboot stuff
for future platforms anyway (see
https://review.coreboot.org/c/coreboot/+/38590/comment/7f384f61_597e79c9/),
so I'm hoping we can eventually get rid of it (and just drag it along
until then by hiding it in vendorcode and trying to maintain
prog_locate_hook() with minimal effort). In the future, we will
hopefully have good and flexible measurement and verification
solutions ready to use (IIRC this was one of the reasons Eltan
implemented the mboot stuff in the first place, because Philipp's
measurement stuff wasn't quite finished back then yet), and then
nobody will feel the need to implement their own thing again.
> Also, regarding your point on gerrit (collecting arguments in this thread) that we don't have duplicate things, look no further than graphics init:
> src/device/oprom/realmode
> src/device/oprom/yabel
> src/drivers/intel/gma
> FSP can do the graphics init, too (and report it in cbtables)

Don't most of those target different hardware generations? Then I
don't really think that's comparable... of course if we're trying to
support hardware with widely different graphics interfaces that may
result in different driver code. But for pure, hardware-independent
feature stuff like verification or measurement, I think we should aim
to converge into a single implementation that can support everyone's
use cases. We don't support two different kinds of timestamp tables or
two dynamic persistent memory allocators (CBMEM) either. (Also, like I
mentioned the problem with this security stuff is just that it needs
to be so tightly integrated with the CBFS stack. In a different, more
isolated area I might consider this less of a problem, but the Eltan
mboot stuff has actually been causing practical problems a couple of
times already, in ways that we mostly already pointed out as concerns
when it was added. I'd prefer to get rid of it long-term, not make it
any more official.)