Deprecating spurious PCI bus master enabling (was Re: Planning the next coreboot release)

[Nico Huber]

Hi,

On 04.11.20 23:21, Angel Pons wrote:
> Please take a look at the preliminary release notes in
>
> Documentation/releases/coreboot-4.13-relnotes.md and add whatever
> happened since 4.12 that is worth mentioning. If unsure, simply push a
> change to Gerrit and have your fellow developers discuss it.

I have a candidate for a deprecation warning: We're currently working
to make all not (clearly) necessary PCI bus master enabling optional.
The added Kconfig option still defaults to `y` [1], for compatibility
reasons. This has security implications and hence I propose to proceed
as follows:

After the follow-up (spring) release,
* remove the option and the code using it, or at least
* reduce its usage to a minimum and change the default to `n`.

This should give people enough time to test (with the option disabled)
and potentially fix their payloads (and OS?). If there are no concerns,
I'll add that to the release notes.

There are cases where enabling PCI bus mastering already in coreboot
might be necessary. For instance, Intel documents for their integrated
graphics to enable it before the initialization sequence (without tel-
ling us why and tests show that it ignores the bit anyway *shrug*).
And also cases where it's clearly necessary, e.g. before hiding a PCI
devices configuration space to put it into ACPI mode (if the OS is not
aware of the device' PCI nature, it can't know that bus mastering needs
to be enabled).

If anybody knows or discovers more cases where it needs to be enabled
in advance by coreboot, please mention it here.

Nico

[1] https://review.coreboot.org/c/coreboot/+/42459

[Nico Huber]

On 10.11.20 16:06, Nico Huber wrote:
> If anybody knows or discovers more cases where it needs to be enabled
> in advance by coreboot, please mention it here.

We just discussed on IRC cases where unfixable OS drivers might need it.
For such cases, it would probably be best to add individual Kconfig
switches for each case. This would make it easier to get rid of the big
switch.

Nico

[ron minnich]

nice idea!

On Tue, Nov 10, 2020 at 7:13 AM Nico Huber  wrote:
> On 10.11.20 16:06, Nico Huber wrote:
> > If anybody knows or discovers more cases where it needs to be enabled
> > in advance by coreboot, please mention it here.
>
> We just discussed on IRC cases where unfixable OS drivers might need it.
> For such cases, it would probably be best to add individual Kconfig
> switches for each case. This would make it easier to get rid of the big
> switch.
>
> Nico
> _______________________________________________
> coreboot mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

We could introduce a Kconfig switch per driver and let the driver handle the bit.
Everything else could be removed. This would make it easier to track the usages.
It would be nice if we could agree on a naming scheme so that all switches are named similar which would make it easier to track the usage.

But there are cases where there is no driver in coreboot for a given PCI device which needs this bit. For now, we (Siemens) handle this cases on mainboard level.
So either we need drivers for these devices (just simply setting the master bit) or we can agree on some kind of exceptions.
I am open to everything.

Werner

-----Ursprüngliche Nachricht-----
Von: ron minnich  
Gesendet: Dienstag, 10. November 2020 16:15
An: Nico Huber 
Cc: coreboot 
Betreff: [coreboot] Re: Deprecating spurious PCI bus master enabling (was Re: Planning the next coreboot release)

nice idea!

On Tue, Nov 10, 2020 at 7:13 AM Nico Huber  wrote:
> On 10.11.20 16:06, Nico Huber wrote:
> > If anybody knows or discovers more cases where it needs to be 
> > enabled in advance by coreboot, please mention it here.
>
> We just discussed on IRC cases where unfixable OS drivers might need it.
> For such cases, it would probably be best to add individual Kconfig 
> switches for each case. This would make it easier to get rid of the 
> big switch.
>
> Nico
> _______________________________________________
> coreboot mailing list -- [e-mail] To unsubscribe send an 
> email to [e-mail]

_______________________________________________
coreboot mailing list -- [e-mail] To unsubscribe send an email to [e-mail]

Deprecating spurious PCI bus master enabling

[Paul Menzel]

Dear coreboot folks,

Am 10.11.20 um 16:23 schrieb [e-mail]:
> We could introduce a Kconfig switch per driver and let the driver
> handle the bit. Everything else could be removed. This would make it
> easier to track the usages. It would be nice if we could agree on a
> naming scheme so that all switches are named similar which would make
> it easier to track the usage.
>
> But there are cases where there is no driver in coreboot for a given
> PCI device which needs this bit. For now, we (Siemens) handle this
> cases on mainboard level. So either we need drivers for these devices
> (just simply setting the master bit) or we can agree on some kind of
> exceptions. I am open to everything.

Two more suggestions:

1.  Log a warning for all those cases, that this might have security 
implications a.nd that it should be made sure, that the payload and OS 
do it themselves.

Warning: Bus master enabled for …. This impacts security, so 
ideally make sure your payload and OS is able to set it themselves.

2.  Make it run-time configurable for easier testing.

Kind regards,

Paul

[Angel Pons]

Hi Werner, Ron, Nico, list,

While it would be great to not have to implement Bus Master
workarounds in coreboot, I guess it's sometimes unavoidable because of
external constraints. I would much prefer to have the actual problem
fixed instead (code assuming Bus Master is enabled by default), but if
that's not possible I can live with some mechanism to enable Bus
Master in coreboot, *as long as all usages are justified*. IMHO,
enabling Bus Master for no reason is bad practice, but keeping code to
enable Bus Master because no one knows what removing it could break is
even worse.

On Tue, Nov 10, 2020 at 3:24 PM [e-mail]
 wrote:
> We could introduce a Kconfig switch per driver

Not per-driver, please!

I'd rather have per-reason switches (reason-options): Kconfig options
whose help-text gives the reason why enabling Bus Master in coreboot
for certain devices is needed. Even if these Kconfig options end up
being scattered across the tree, it's easier to see the relationship
between multiple instances of Bus Master being set. Plus, if the
actual bug is eventually fixed, it's very easy to drop the
corresponding reason-option without breaking any other reason-options.
> and let the driver handle the bit.
> Everything else could be removed. This would make it easier to track the usages.
> It would be nice if we could agree on a naming scheme so that all switches are named similar which would make it easier to track the usage.
>
> But there are cases where there is no driver in coreboot for a given PCI device which needs this bit. For now, we (Siemens) handle this cases on mainboard level.
> So either we need drivers for these devices (just simply setting the master bit) or we can agree on some kind of exceptions.
> I am open to everything.

This won't be a problem if using per-reason Kconfig options. I would
still use these reason-options though, even when the option always
gets selected (e.g. selected by the mainboard's Kconfig): this is to
justify each instance of Bus Master being set with the corresponding
reason-options.
> Werner
>
> -----Ursprüngliche Nachricht-----
> Von: ron minnich 
> Gesendet: Dienstag, 10. November 2020 16:15
> An: Nico Huber 
> Cc: coreboot 
> Betreff: [coreboot] Re: Deprecating spurious PCI bus master enabling (was Re: Planning the next coreboot release)
>
> nice idea!
>
> On Tue, Nov 10, 2020 at 7:13 AM Nico Huber  wrote:
> > On 10.11.20 16:06, Nico Huber wrote:
> > > If anybody knows or discovers more cases where it needs to be
> > > enabled in advance by coreboot, please mention it here.
> >
> > We just discussed on IRC cases where unfixable OS drivers might need it.
> > For such cases, it would probably be best to add individual Kconfig
> > switches for each case. This would make it easier to get rid of the
> > big switch.
> >
> > Nico
> > _______________________________________________
> > coreboot mailing list -- [e-mail] To unsubscribe send an
> > email to [e-mail]
>
> coreboot mailing list -- [e-mail] To unsubscribe send an email to [e-mail]
> _______________________________________________
> coreboot mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

Best regards,
Angel