[PATCH] optionrom: disallow int19 redirect for pnp roms.
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
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
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
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
@@ -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
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
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
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
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
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
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
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
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
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
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
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
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
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(); }
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
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/JZTBQWM... which is untested so far and not committed yet because of that. Can you check whenever that one works as intended? thanks, Gerd
participants (4)
-
Gerd Hoffmann -
Kevin O'Connor -
Laszlo Ersek -
Matt DeVillier