[SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.

Gerd Hoffmann posted 1 patch 5 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20181127121038.16482-1-kraxel@redhat.com
There is a newer version of this series
src/optionroms.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
[SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
Check whenever pnp roms attempt to redirect int19, and in case it does
log a message and undo the redirect.

A pnp rom should not need this, we have BEVs and BCVs for that.
Nevertheless there are roms in the wild which are redirecting int19.
At least some BIOS implementations for physical hardware have a config
option in the setup to allow/disallow int19 redirections, so just not
allowing this seems to be the way to deal with this situation.

Buglink: https://bugzilla.redhat.com//show_bug.cgi?id=1642135
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 src/optionroms.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/optionroms.c b/src/optionroms.c
index fc992f649f..4ec5504ca9 100644
--- a/src/optionroms.c
+++ b/src/optionroms.c
@@ -8,6 +8,7 @@
 #include "bregs.h" // struct bregs
 #include "config.h" // CONFIG_*
 #include "farptr.h" // FLATPTR_TO_SEG
+#include "biosvar.h" // GET_IVT
 #include "hw/pci.h" // pci_config_readl
 #include "hw/pcidevice.h" // foreachpci
 #include "hw/pci_ids.h" // PCI_CLASS_DISPLAY_VGA
@@ -136,9 +137,24 @@ init_optionrom(struct rom_header *rom, u16 bdf, int isvga)
 
     tpm_option_rom(newrom, rom->size * 512);
 
-    if (isvga || get_pnp_rom(newrom))
+    struct pnp_data *pnp = get_pnp_rom(newrom);
+    if (isvga || pnp) {
+        struct segoff_s old19, new19;
         // Only init vga and PnP roms here.
+        old19 = GET_IVT(0x19);
         callrom(newrom, bdf);
+        new19 = GET_IVT(0x19);
+        if (old19.seg != new19.seg ||
+            old19.offset != new19.offset) {
+            dprintf(1, "WARNING! rom tried to hijack int19 "
+                    "(vec %04x:%04x, pnp %s, bev %s, bvc %s)\n",
+                    new19.seg, new19.offset,
+                    pnp             ? "yes" : "no",
+                    pnp && pnp->bev ? "yes" : "no",
+                    pnp && pnp->bcv ? "yes" : "no");
+            SET_IVT(0x19, old19);
+        }
+    }
 
     return rom_confirm(newrom->size * 512);
 }
-- 
2.9.3


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Tue, Nov 27, 2018 at 01:10:38PM +0100, Gerd Hoffmann wrote:
> Check whenever pnp roms attempt to redirect int19, and in case it does
> log a message and undo the redirect.
> 
> A pnp rom should not need this, we have BEVs and BCVs for that.
> Nevertheless there are roms in the wild which are redirecting int19.
> At least some BIOS implementations for physical hardware have a config
> option in the setup to allow/disallow int19 redirections, so just not
> allowing this seems to be the way to deal with this situation.
> 
> Buglink: https://bugzilla.redhat.com//show_bug.cgi?id=1642135

That is very odd.  I'm pretty sure iPXE normally does register itself
as a BEV - any idea why it's now hooking int19?

I'm leery of making a change like this, because there's a good chance
it will break something in some other obscure software.  I think
fixing this in iPXE would be preferable if possible.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
On Tue, Nov 27, 2018 at 09:19:09PM -0500, Kevin O'Connor wrote:
> On Tue, Nov 27, 2018 at 01:10:38PM +0100, Gerd Hoffmann wrote:
> > Check whenever pnp roms attempt to redirect int19, and in case it does
> > log a message and undo the redirect.
> > 
> > A pnp rom should not need this, we have BEVs and BCVs for that.
> > Nevertheless there are roms in the wild which are redirecting int19.
> > At least some BIOS implementations for physical hardware have a config
> > option in the setup to allow/disallow int19 redirections, so just not
> > allowing this seems to be the way to deal with this situation.
> > 
> > Buglink: https://bugzilla.redhat.com//show_bug.cgi?id=1642135
> 
> That is very odd.  I'm pretty sure iPXE normally does register itself
> as a BEV - any idea why it's now hooking int19?

It's not ipxe.

It is the rom of a intel nic, attached to a guest via pci passthrough.
It does both, register bev and hook int19.  No clue why.  The only
reason I can think of is backward compatibility to firmware so old that
it doesn't know pnp roms.  Which is a silly thing in pci express
hardware.  Maybe they carry forward that code since decades ...

> I'm leery of making a change like this, because there's a good chance
> it will break something in some other obscure software.

I've added a rather verbose message printing some information about the
rom because of that.

> I think fixing this in iPXE would be preferable if possible.

See above. ipxe doesn't need fixing.

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Laszlo Ersek 5 years, 4 months ago
On 11/28/18 07:24, Gerd Hoffmann wrote:
> On Tue, Nov 27, 2018 at 09:19:09PM -0500, Kevin O'Connor wrote:
>> On Tue, Nov 27, 2018 at 01:10:38PM +0100, Gerd Hoffmann wrote:
>>> Check whenever pnp roms attempt to redirect int19, and in case it does
>>> log a message and undo the redirect.
>>>
>>> A pnp rom should not need this, we have BEVs and BCVs for that.
>>> Nevertheless there are roms in the wild which are redirecting int19.
>>> At least some BIOS implementations for physical hardware have a config
>>> option in the setup to allow/disallow int19 redirections, so just not
>>> allowing this seems to be the way to deal with this situation.
>>>
>>> Buglink: https://bugzilla.redhat.com//show_bug.cgi?id=1642135
>>
>> That is very odd.  I'm pretty sure iPXE normally does register itself
>> as a BEV - any idea why it's now hooking int19?
> 
> It's not ipxe.
> 
> It is the rom of a intel nic, attached to a guest via pci passthrough.
> It does both, register bev and hook int19.  No clue why.  The only
> reason I can think of is backward compatibility to firmware so old that
> it doesn't know pnp roms.  Which is a silly thing in pci express
> hardware.  Maybe they carry forward that code since decades ...

Right. Before I raised my short question about *not* short-circuiting
get_pnp_rom() with "isvga" set, I had read through the BZ, and I was
*very* tempted to say "this is what's wrong with our industry". :) The
oprom in question is mind-bogglingly broken, from the discussion /
analysis in the BZ.

(I'm sure somewhere deep in an internal bug tracking system at the card
vendor there is a ticket about some broken platform BIOS where the BEV
wouldn't work, and they had to hook Int19h.)

>> I'm leery of making a change like this, because there's a good chance
>> it will break something in some other obscure software.
> 
> I've added a rather verbose message printing some information about the
> rom because of that.
> 
>> I think fixing this in iPXE would be preferable if possible.
> 
> See above. ipxe doesn't need fixing.

I support the addition of this "safety code", and I tend to agree (with
the BZ discussion) that making it *dynamically* configurable could be
difficult and/or overkill.

Kevin, would you feel easier about the Int19h vector restoration if it
were controlled by a new, static, config knob?

Thanks,
Laszlo

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Wed, Nov 28, 2018 at 12:14:07PM +0100, Laszlo Ersek wrote:
> Right. Before I raised my short question about *not* short-circuiting
> get_pnp_rom() with "isvga" set, I had read through the BZ, and I was
> *very* tempted to say "this is what's wrong with our industry". :) The
> oprom in question is mind-bogglingly broken, from the discussion /
> analysis in the BZ.
> 
> (I'm sure somewhere deep in an internal bug tracking system at the card
> vendor there is a ticket about some broken platform BIOS where the BEV
> wouldn't work, and they had to hook Int19h.)

Right - fundamental to X86 booting is the idea that firmware
developers write code, PC manufacturers write code, peripheral
manufacturers write code, and only users test all the code together.
It's a broken workflow.  It's been nearly 40 years and X86 is still
stuck in this broken workflow.

> >> I'm leery of making a change like this, because there's a good chance
> >> it will break something in some other obscure software.
> > 
> > I've added a rather verbose message printing some information about the
> > rom because of that.
> > 
> >> I think fixing this in iPXE would be preferable if possible.
> > 
> > See above. ipxe doesn't need fixing.
> 
> I support the addition of this "safety code", and I tend to agree (with
> the BZ discussion) that making it *dynamically* configurable could be
> difficult and/or overkill.
> 
> Kevin, would you feel easier about the Int19h vector restoration if it
> were controlled by a new, static, config knob?

If we could do it safely that would be fine.  My fear is that it
introduces a regression.  A new config option would be okay, but it
doesn't sound like that will help, as it seems that once one narrows
down the problem to a bad behaving optionrom, one could just as easily
block that optionrom instead..

I'm not sure what the best choice is.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
  Hi,

> > Kevin, would you feel easier about the Int19h vector restoration if it
> > were controlled by a new, static, config knob?
> 
> If we could do it safely that would be fine.  My fear is that it
> introduces a regression.  A new config option would be okay, but it
> doesn't sound like that will help, as it seems that once one narrows
> down the problem to a bad behaving optionrom, one could just as easily
> block that optionrom instead..

Well, as already mentioned, physical hardware seems to solve that with
an config option to enable/disable hooking int19.  Look here for
example:

https://superuser.com/questions/1000339/interrupt-19-capture-bios-option

(Which effectively gives the user a tool to deal with the mess the broken
 development workflow created).

Thats why I picked the path to just not allow int19 hooks.  For pnp
roms, non-pnp roms are still allowed to do that.  And we have the option
to refine the logic if needed, check whenever the pnp rom has a bev or
bcv for example.

I'm also trying to avoid a config option.  Asking the user to deal with
the mess isn't a good way IMHO ...

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Thu, Nov 29, 2018 at 09:25:14AM +0100, Gerd Hoffmann wrote:
> > > Kevin, would you feel easier about the Int19h vector restoration if it
> > > were controlled by a new, static, config knob?
> > 
> > If we could do it safely that would be fine.  My fear is that it
> > introduces a regression.  A new config option would be okay, but it
> > doesn't sound like that will help, as it seems that once one narrows
> > down the problem to a bad behaving optionrom, one could just as easily
> > block that optionrom instead..
> 
> Well, as already mentioned, physical hardware seems to solve that with
> an config option to enable/disable hooking int19.  Look here for
> example:
> 
> https://superuser.com/questions/1000339/interrupt-19-capture-bios-option
> 
> (Which effectively gives the user a tool to deal with the mess the broken
>  development workflow created).

Right, but I wonder if it being a bios option indicates that it
sometimes causes regressions.  I suppose we could do the same -
recapture int 19 by default and add an obscure option to disable that
behavior.

> Thats why I picked the path to just not allow int19 hooks.  For pnp
> roms, non-pnp roms are still allowed to do that.  And we have the option
> to refine the logic if needed, check whenever the pnp rom has a bev or
> bcv for example.

That's a good point - a PnP rom really shouldn't have hooked int19, so
recapturing it should not be that bad.

Do you know, for this particular optionrom, if int19 is recaptured and
the user then chooses it from the boot menu, does it continue to work?

> I'm also trying to avoid a config option.  Asking the user to deal with
> the mess isn't a good way IMHO ...

Agreed.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
  Hi,

> Do you know, for this particular optionrom, if int19 is recaptured and
> the user then chooses it from the boot menu, does it continue to work?

I've asked them to test that.

I expect it continues to work given that at least some firmware
implementations have a knob to disallow int19 hooks, but you never
know ...

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
On Fri, Nov 30, 2018 at 12:53:38PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Do you know, for this particular optionrom, if int19 is recaptured and
> > the user then chooses it from the boot menu, does it continue to work?
> 
> I've asked them to test that.

Feedback arrived:  Yes, picking a nic from the boot menu continues to
work with recaptured int19.

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Wed, Dec 05, 2018 at 07:55:58AM +0100, Gerd Hoffmann wrote:
> On Fri, Nov 30, 2018 at 12:53:38PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Do you know, for this particular optionrom, if int19 is recaptured and
> > > the user then chooses it from the boot menu, does it continue to work?
> > 
> > I've asked them to test that.
> 
> Feedback arrived:  Yes, picking a nic from the boot menu continues to
> work with recaptured int19.

Okay, thanks.  If you think recapturing int19 is the best way forward
then I'm okay with that.

I do think it may be prudent to limit the test a little more (just in
an abundance of caution).  For example, something like the below.

-Kevin


--- a/src/optionroms.c
+++ b/src/optionroms.c
@@ -327,8 +334,12 @@ init_pcirom(struct pci_device *pci, int isvga, u64 *sources)
     if (! rom)
         // No ROM present.
         return;
+    int irq_was_captured = boot_irq_captured();
     setRomSource(sources, rom, RS_PCIROM | (u32)pci);
     init_optionrom(rom, pci->bdf, isvga);
+    if (boot_irq_captured() && !irq_was_captured && !file && !isvga)
+        // This PCI rom is misbehaving - recapture the boot irqs
+        recapture_boot_irq();
 }
 
 

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
[SeaBIOS] Re: [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Matt DeVillier 4 years, 11 months ago
reviving this, as I've run into this issue with some Intel 10Gbe NICs on a
board I've been testing (Intel Corporation Ethernet Connection X552 10 GbE
SFP+ [8086:15ac]).  This patch resolves the issue for me

On Wed, Dec 5, 2018 at 4:49 PM Kevin O'Connor <kevin@koconnor.net> wrote:

> On Wed, Dec 05, 2018 at 07:55:58AM +0100, Gerd Hoffmann wrote:
> > On Fri, Nov 30, 2018 at 12:53:38PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > > Do you know, for this particular optionrom, if int19 is recaptured
> and
> > > > the user then chooses it from the boot menu, does it continue to
> work?
> > >
> > > I've asked them to test that.
> >
> > Feedback arrived:  Yes, picking a nic from the boot menu continues to
> > work with recaptured int19.
>
> Okay, thanks.  If you think recapturing int19 is the best way forward
> then I'm okay with that.
>
> I do think it may be prudent to limit the test a little more (just in
> an abundance of caution).  For example, something like the below.
>
> -Kevin
>
>
> --- a/src/optionroms.c
> +++ b/src/optionroms.c
> @@ -327,8 +334,12 @@ init_pcirom(struct pci_device *pci, int isvga, u64
> *sources)
>      if (! rom)
>          // No ROM present.
>          return;
> +    int irq_was_captured = boot_irq_captured();
>      setRomSource(sources, rom, RS_PCIROM | (u32)pci);
>      init_optionrom(rom, pci->bdf, isvga);
> +    if (boot_irq_captured() && !irq_was_captured && !file && !isvga)
> +        // This PCI rom is misbehaving - recapture the boot irqs
> +        recapture_boot_irq();
>  }
>
>
>
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS@seabios.org
> https://mail.coreboot.org/mailman/listinfo/seabios
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 4 years, 11 months ago
On Tue, May 14, 2019 at 03:09:37PM -0500, Matt DeVillier wrote:
> reviving this, as I've run into this issue with some Intel 10Gbe NICs on a
> board I've been testing (Intel Corporation Ethernet Connection X552 10 GbE
> SFP+ [8086:15ac]).  This patch resolves the issue for me

There is a v2 of that patch:
  https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/JZTBQWMUTC2FZLZUGLEYDNCRPBIEZ5MG/

which is untested so far and not committed yet because of that.
Can you check whenever that one works as intended?

thanks,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Laszlo Ersek 5 years, 4 months ago
On 11/28/18 16:51, Kevin O'Connor wrote:
> On Wed, Nov 28, 2018 at 12:14:07PM +0100, Laszlo Ersek wrote:
>> Right. Before I raised my short question about *not* short-circuiting
>> get_pnp_rom() with "isvga" set, I had read through the BZ, and I was
>> *very* tempted to say "this is what's wrong with our industry". :) The
>> oprom in question is mind-bogglingly broken, from the discussion /
>> analysis in the BZ.
>>
>> (I'm sure somewhere deep in an internal bug tracking system at the card
>> vendor there is a ticket about some broken platform BIOS where the BEV
>> wouldn't work, and they had to hook Int19h.)
> 
> Right - fundamental to X86 booting is the idea that firmware
> developers write code, PC manufacturers write code, peripheral
> manufacturers write code, and only users test all the code together.
> It's a broken workflow.  It's been nearly 40 years and X86 is still
> stuck in this broken workflow.
> 
>>>> I'm leery of making a change like this, because there's a good chance
>>>> it will break something in some other obscure software.
>>>
>>> I've added a rather verbose message printing some information about the
>>> rom because of that.
>>>
>>>> I think fixing this in iPXE would be preferable if possible.
>>>
>>> See above. ipxe doesn't need fixing.
>>
>> I support the addition of this "safety code", and I tend to agree (with
>> the BZ discussion) that making it *dynamically* configurable could be
>> difficult and/or overkill.
>>
>> Kevin, would you feel easier about the Int19h vector restoration if it
>> were controlled by a new, static, config knob?
> 
> If we could do it safely that would be fine.  My fear is that it
> introduces a regression.  A new config option would be okay, but it
> doesn't sound like that will help, as it seems that once one narrows
> down the problem to a bad behaving optionrom, one could just as easily
> block that optionrom instead..

Do you mean that a "blacklist" should be added (a static array of
checksums, of known-bad ROM images)?

Thanks,
Laszlo

> I'm not sure what the best choice is.
> 
> -Kevin
> 


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Wed, Nov 28, 2018 at 06:50:50PM +0100, Laszlo Ersek wrote:
> On 11/28/18 16:51, Kevin O'Connor wrote:
> > If we could do it safely that would be fine.  My fear is that it
> > introduces a regression.  A new config option would be okay, but it
> > doesn't sound like that will help, as it seems that once one narrows
> > down the problem to a bad behaving optionrom, one could just as easily
> > block that optionrom instead..
> 
> Do you mean that a "blacklist" should be added (a static array of
> checksums, of known-bad ROM images)?

If I understand the bugzilla report correctly, it would be possible to
avoid this issue by using <rom bar='off'/> in libvirt.  It appears the
issue is identifying the problem and then there are further issues
with changing that config.

Implementing a default blacklist is a thought that I had.  If we feel
the software we control is working as intended and it is the optionrom
that is broken, then perhaps the focus should be on not running that
optionrom.  (Effectively changing the default to run only known good
optionroms on pci passthrough.)  I don't think SeaBIOS would be the
place to maintain a blacklist/whitelist though, so it's an easy
proposal for me to make..  I understand if it is not viable.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Laszlo Ersek 5 years, 4 months ago
On 11/28/18 19:33, Kevin O'Connor wrote:
> On Wed, Nov 28, 2018 at 06:50:50PM +0100, Laszlo Ersek wrote:
>> On 11/28/18 16:51, Kevin O'Connor wrote:
>>> If we could do it safely that would be fine.  My fear is that it
>>> introduces a regression.  A new config option would be okay, but it
>>> doesn't sound like that will help, as it seems that once one narrows
>>> down the problem to a bad behaving optionrom, one could just as easily
>>> block that optionrom instead..
>>
>> Do you mean that a "blacklist" should be added (a static array of
>> checksums, of known-bad ROM images)?
> 
> If I understand the bugzilla report correctly, it would be possible to
> avoid this issue by using <rom bar='off'/> in libvirt.  It appears the
> issue is identifying the problem and then there are further issues
> with changing that config.
> 
> Implementing a default blacklist is a thought that I had.  If we feel
> the software we control is working as intended and it is the optionrom
> that is broken, then perhaps the focus should be on not running that
> optionrom.  (Effectively changing the default to run only known good
> optionroms on pci passthrough.)  I don't think SeaBIOS would be the
> place to maintain a blacklist/whitelist though, so it's an easy
> proposal for me to make..  I understand if it is not viable.

So, if I understand correctly:

- doing something generic in SeaBIOS is too risky / heavy-handed,
- discriminating individual oproms in SeaBIOS is out of scope.

Well... If we put it like that, I can't say that I disagree. I'll try to
carry this over to the RHBZ.

Thanks,
Laszlo

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Wed, Nov 28, 2018 at 11:16:57PM +0100, Laszlo Ersek wrote:
> So, if I understand correctly:
> 
> - doing something generic in SeaBIOS is too risky / heavy-handed,
> - discriminating individual oproms in SeaBIOS is out of scope.
> 
> Well... If we put it like that, I can't say that I disagree. I'll try to
> carry this over to the RHBZ.

I'm not against changing SeaBIOS.  I'd say the current state is "in
discussion".

Thanks,
-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Kevin O'Connor 5 years, 4 months ago
On Wed, Nov 28, 2018 at 07:24:21AM +0100, Gerd Hoffmann wrote:
> On Tue, Nov 27, 2018 at 09:19:09PM -0500, Kevin O'Connor wrote:
> > That is very odd.  I'm pretty sure iPXE normally does register itself
> > as a BEV - any idea why it's now hooking int19?
> 
> It's not ipxe.

Ah, okay.  The bugzilla entry is confusing for those without access to
the full report.

> It is the rom of a intel nic, attached to a guest via pci passthrough.
> It does both, register bev and hook int19.  No clue why.  The only
> reason I can think of is backward compatibility to firmware so old that
> it doesn't know pnp roms.  Which is a silly thing in pci express
> hardware.  Maybe they carry forward that code since decades ...

It's also possible that the nic attempted to verify the bios was pnp
compatible, but seabios failed that check for some reason.  More
likely that the nic optionrom is just broken though.

> > I'm leery of making a change like this, because there's a good chance
> > it will break something in some other obscure software.
> 
> I've added a rather verbose message printing some information about the
> rom because of that.

Unfortunately, I fear no one will see that warning in practice -
instead I fear a problem would appear as an obscure regression.

FWIW, the following is from the "BIOS Boot Specification" v1.01:

============================================

Legacy IPL devices will be allowed to take control of the system (via
hooking interrupts) in both Legacy and PnP systems. The Plug and Play
BIOS specification recommends that Legacy devices that hook a
bootstrap interrupt such as INT 19h, 18h, or 13h have the interrupt
re-captured by the BIOS. This is not done because grabbing an
interrupt vector back after a device has hooked it can produce
unpredictable results.  Further, by allowing the card to take control,
the behavior of these Legacy cards will be the same on both PnP and
Legacy machines.

============================================

Which I read as an indicator that recapturing the int 0x19 vector was
known to cause problems.

> > I think fixing this in iPXE would be preferable if possible.
> 
> See above. ipxe doesn't need fixing.

In comment #29 and #32 of the above bugzilla, it is mentioned there
are other workarounds - do those not work in practice?  It sounds like
maybe this particular rom should just be blacklisted somehow.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
  Hi,

> Unfortunately, I fear no one will see that warning in practice -
> instead I fear a problem would appear as an obscure regression.

Hmm, yes, have to agree to that.  Took me a while to figure what was
going on in this case, and regressions causes by this have a good chance
to be equally obscure.

> > See above. ipxe doesn't need fixing.
> 
> In comment #29 and #32 of the above bugzilla, it is mentioned there
> are other workarounds - do those not work in practice?  It sounds like
> maybe this particular rom should just be blacklisted somehow.

The main advantage of the seabios patch would be that it works without
requiring the user to configure something.  Also figuring that the
optionrom is the problem (and not the seabios bootorder logic) isn't
that easy.

Changing the guest configuration allows you to (a) just disable the rom,
or (b) use ipxe rom instead.

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Laszlo Ersek 5 years, 4 months ago
On 11/27/18 13:10, Gerd Hoffmann wrote:
> Check whenever pnp roms attempt to redirect int19, and in case it does
> log a message and undo the redirect.
> 
> A pnp rom should not need this, we have BEVs and BCVs for that.
> Nevertheless there are roms in the wild which are redirecting int19.
> At least some BIOS implementations for physical hardware have a config
> option in the setup to allow/disallow int19 redirections, so just not
> allowing this seems to be the way to deal with this situation.
> 
> Buglink: https://bugzilla.redhat.com//show_bug.cgi?id=1642135
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  src/optionroms.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/optionroms.c b/src/optionroms.c
> index fc992f649f..4ec5504ca9 100644
> --- a/src/optionroms.c
> +++ b/src/optionroms.c
> @@ -8,6 +8,7 @@
>  #include "bregs.h" // struct bregs
>  #include "config.h" // CONFIG_*
>  #include "farptr.h" // FLATPTR_TO_SEG
> +#include "biosvar.h" // GET_IVT
>  #include "hw/pci.h" // pci_config_readl
>  #include "hw/pcidevice.h" // foreachpci
>  #include "hw/pci_ids.h" // PCI_CLASS_DISPLAY_VGA
> @@ -136,9 +137,24 @@ init_optionrom(struct rom_header *rom, u16 bdf, int isvga)
>  
>      tpm_option_rom(newrom, rom->size * 512);
>  
> -    if (isvga || get_pnp_rom(newrom))
> +    struct pnp_data *pnp = get_pnp_rom(newrom);
> +    if (isvga || pnp) {
> +        struct segoff_s old19, new19;
>          // Only init vga and PnP roms here.
> +        old19 = GET_IVT(0x19);
>          callrom(newrom, bdf);
> +        new19 = GET_IVT(0x19);
> +        if (old19.seg != new19.seg ||
> +            old19.offset != new19.offset) {
> +            dprintf(1, "WARNING! rom tried to hijack int19 "
> +                    "(vec %04x:%04x, pnp %s, bev %s, bvc %s)\n",
> +                    new19.seg, new19.offset,
> +                    pnp             ? "yes" : "no",
> +                    pnp && pnp->bev ? "yes" : "no",
> +                    pnp && pnp->bcv ? "yes" : "no");
> +            SET_IVT(0x19, old19);
> +        }
> +    }
>  
>      return rom_confirm(newrom->size * 512);
>  }
> 

Is it OK to call get_pnp_rom() when "isvga" is set?

Thanks
Laszlo

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] optionrom: disallow int19 redirect for pnp roms.
Posted by Gerd Hoffmann 5 years, 4 months ago
> > @@ -136,9 +137,24 @@ init_optionrom(struct rom_header *rom, u16 bdf, int isvga)
> >  
> >      tpm_option_rom(newrom, rom->size * 512);
> >  
> > -    if (isvga || get_pnp_rom(newrom))
> > +    struct pnp_data *pnp = get_pnp_rom(newrom);
> > +    if (isvga || pnp) {

> Is it OK to call get_pnp_rom() when "isvga" is set?

Yes.  In case the rom isn't a pnp rom you just get back a NULL pointer.

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios