-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider working with fwupd? #32
Comments
Hello! Thanks for the interest. I'll share a bit of background too:
I've got some patches in-flight for the AST2600 that might also interest you: master...ast2600 - expect iterations on that series in the days to come :) Anyway, I'm keen on the idea of integrating culvert's functionality into fwupd. I'll take a look at how fwupd currently implements these tests and have a bit of a think about what the integration path might look like. I'd certainly like your thoughts on that too. Feel free to email me directly and we can chat that way, and maybe use that to bootstrap a zoom session. Thanks for getting in touch! |
Hey Andrew, I was talking to Alex from Eclypsium just a few weeks ago; him and I sync fairly frequently and swap ideas. I'm pretty sure he’s already got a keen eye on fwupd too. :)
Okay, that would be amazing, but I do see a few potential blockers. Firstly, the licence of fwupd is LGPLv2+, although a few source files from OEMs are dual-licenced, e.g. “LGPLv2+ OR MIT” – I don’t know how you would feel about “LGPLv2+ OR Apache-2.0” if we copy-pasted bits of code from culvert into fwupd. I think the Apache-2.0 and LGPLv2+ licences are broadly the same with a few subtle differences but I’m certainly not an expert in this stuff. The second is how fwupd needs to run with almost no additional deps – each additional dep means the number of people using the feature goes down two orders of magnitude. So although we can’t depend on things like libfdt -- although we do have a homegrown DeviceTree parser and a FIT parser included in fwupd right now: https://github.com/fwupd/fwupd/blob/main/libfwupdplugin/fu-fdt-firmware.c The last potential blocker is that we use GObject C in fwupd, rather than “plain” C – so things are objects, signals, properties etc rather that the kernel current style of culvert. e.g. in fwupd parlance, we have a backend that emits device objects with properties, and each test is a HSI attribute with result attributes, rather than the simple (but easy to understand) structure of culvert. The pragmatic choice might be for fwupd to “steal ideas” from culvert with attribution, and we work together on the high-level tests themselves – maybe even code assuming the license stuff works out of course. I don’t think it makes sense to depend on having a bleeding edge fwupd binary to be able to do low-level AST2x00 bring-up on a new board, for instance -- and so I don't think absorbing culvert is a nice or necessary thing to suggest. I guess the most common place fwupd runs right now is when it’s running on the host OS, and so we’d need to use LPC & PCIe to talk to the BMC but keep in the back of our minds we’ll need to do the I’d also like to know of the best way to play with this stuff; I have an expensive Lenovo server here with ASPEED Pilot 4 BMC, but I’m guessing those are not going to be supported in the same way. I’ve also got a evb-ast2500 on my desk which we used for the fwupd-on-BMC demo – which works with the I guess one thing we could do right now is actually flesh out the culvert probe stuff to include a ton more data, perhaps even creating HSI attributes or a JSON export for each of them. For instance, creating files like https://github.com/fwupd/fwupd/blob/main/docs/hsi-tests.d/org.fwupd.hsi.Mei.ManufacturingMode.json that get built into the docs like https://fwupd.github.io/libfwupdplugin/hsi.html#me-not-in-manufacturing-mode make this stuff into an official standard we can point vendors at – but you’re obviously the expert here. I guess one first action for me would be to move the fdt-parsing code out of the existing |
Quite a bit to work through there :) I took a look at the fwupd codebase after you opened the issue and I tend to agree the way forward is one of two extremes:
I'm not sure how 2 plays into your "each additional dep means the number of people using the feature goes down two orders of magnitude" issue - arguably it would be a dependency, but not in a library sense. I'm not sure if the distinction matters. Shipping culvert as a separate binary avoids us having to actually resolve the licensing question, and we could add meson options to trim functionality down to meet the requirements of fwupd. Adding a HSI reporting mode to culvert would be pretty neat, though from what I've seen the HSI JSON documents you have are more about enumerating and describing the attributes rather than measuring? Do you have a schema for measurement results? Regardless, I think we could cook up some descriptions based on the text in CVE-2019-6260. 1 is obviously more work, but gives a much tighter integration into fwupd and restricts the scope of behaviour to at most fill fwupd's requirements. Was flashing a new BMC image ever a goal (asking due to some of the discussion on Mastodon)? There's obviously tension there between measuring the bridge state to assess security posture and requiring the bridges be available for flashing purposes, bypassing any authentication/authorisation on the BMC's part. As for playing with stuff, I think the other thread on Mastodon is going to be the most help there. Aspeed are also quite responsive on the OpenBMC Discord and might be able to give you some pointers (maybe it's possible to rig up an EVB in a useful configuration - I don't have one at hand). Anyway, I'm happy to help out either way where I can! |
Ya, sorry! I'm passionate about this firmware stuff, I could talk for hours about this stuff :)
I'm less keen on this; various vendors have tried to convince us to exec random binaries for enumeration, firmware writing and security validation, and I think in all honesty this would be horrific from a "keeping it all working in CI" point of view, and greatly complicates things like project maintenance, device lifecycle, caching and resource locking. It also makes progress reporting almost impossible and also selinux policy much harder to satisfy. I think it would be a bad precedent and would open the floodgates tbh.
Nobody has asked for it, but if we support 69 different (literally) ways to flash hardware, one more is no biggie.
We do both, and even try to fix the problems if that's possible. Obviously not in this case, but it's at least an option. e.g. More interesting for stuff like Red Hat Insights is the JSON output, which culvert could certainly copy, e.g.
I'd appreciate your edits and comments on https://docs.google.com/document/d/1Ib_1hwxCnfe-H0d7jeBOi38NYyTHIzEi66OcU61xLY4/edit?usp=sharing -- I think it makes a lot of sense to agree on the failures and the severities. Maybe some of those don't make sense as HSI attrs, ideas welcome. |
Yeah, many good concerns there. Let's concentrate on getting the bits we need implemented directly in fwupd.
Right, that's the output that I was asking about. From the brief look I took at the JSON documents in fwupd.git it wasn't obvious that e.g.
Yep, let me have a read through and provide some feedback. |
Okay, I've left a bunch of comments on the google doc. Let me know what you think. |
I think a good starting point would be to add the checks for the devmem bridge, e.g. Read DWORD HICRB (0x100h) and check that bit 6 is set -- I've added these sections to the doc -- any help there most welcome. If I understand the culvert code correctly the devmem bridge isn't doing any of the report checks either -- i.e. we need the functionality there too. |
I think a good starting point would be to add the checks for the devmem bridge, e.g. Read DWORD HICRB (0x100h) and check that bit 6 is set -- I've added these sections to the doc -- any help there most welcome. If I understand the culvert code correctly the devmem bridge isn't doing any of the report checks either -- i.e. we need the functionality there too. I'm somewhat sceptical that server OSs are going to let us use all the bridges -- i.e. with kernel lockdown enabled the |
Right, let me fill those out. It made me think about whether there's a better way to describe all the bits so that you could dump them out as reference. That would make your job here a bit easier. However, I think it would require a bit of rearchitecting as I've encoded this info in the code rather than defining it statically.
Can you clarify what you mean here? Is it that you'd like to see a report of whether the
Yeah, all valid concerns. The latter concern is the reason for the warning in caps at the top of the README :) Getting back to Stepping back for a moment, I guess we need to be explicit about what we're trying to establish. Are we trying to establish whether the BMC is vulnerable with no assumptions about the access method on the host side (e.g. bare-metal hosting or host kernel compromise are in-scope), or are we trying to establish vulnerability to the host userspace (assuming root)? I presume the former. If the host kernel is locking down userspace access then I can't see how we can escape having help from the kernel to establish the presence of the vulnerabilities. |
I've added a table describing all the bits in each SoC's physical address space for |
Nah, I mean if you do
Can you remap the BAR when the kernel is locked down?
For the first cut, I want to run fwupd on the BMC as part of OpenBMC and then output a report on what problems there are. I think working on the PCI-to-LPC bit so we can scan from the host OS is probably phase 2.
I think that's a really good start, more certainly welcome! I've got some test code that should be ready for some rough review next week -- i.e. mmap /dev/mem, read 0x1e6e2070 and look for the bit etc. i.e. a really simple first cut. |
This should work. Is it not working for you? I just checked on an AST2600 and it provides this output:
I haven't poked much at lockdown tbh so I went looking. It seems you can't map the BAR into userspace when lockdown is sufficiently enforced:
Sounds good.
Okay. Given you listed BMC and host access separate I wasn't quite sure whether that table was exactly what you were after. Since it's at least useful, we can work to add that table for the remaining HSIs. As a point of reference, anything that calls
So if you want to get a head-start you can dig in that way, otherwise I'll plug away at adding the tables when I have a moment. |
Aha, I didn't know that would work when local. Isn't devmem going to work in all cases, and possibly be a more reliable way to retrieve the information? Or do you think we should always be using pciectl in fwupd when running on the BMC rather than devmem? |
Ah, I think we're running up against two things here:
Let me try to fix 2 properly, however as a brief explanation the codebase is split into approximately three parts: a. The stuff under Considering a. above, the source under
Host bridge drivers are registered in the code via the Reads and writes on the AHB are handy, however, what's even more handy is pairing this capability with the knowledge of how the AHB address space is laid out. The layout of each SoC's AHB is described by the devicetrees in The drivers for the devices in the SoC's address space live in
Enter the naming problem :) I needed some way to differentiate between e.g. Rounding it out, With this structure it's the case that however we drive accesses on the BMC AHB we can easily inspect the configuration of all the bridges, and hence Finally, returning to your comment:
From the above the output
Does that help? |
Okay, I've added the rest of the tables and covered the interesting configuration bits for all of the AST2400, AST2500 and AST2600 in the shared google doc. That should be a decent start. |
Hi Andrew!
Thanks for reaching out on Twitter, this looks exactly like the thing I'm trying to do. I'm really in awe of what you've done in Culvert so far, and I have a feeling I'll have a lot more questions in the future! Some background:
Anyway, this issue is too long already, and is probably actually a discussion -- so if you'd like to move this to email or even a quick zoom call let me know. I'm working for Red Hat in the UK if that helps. Thanks for any ideas or feedback.
The text was updated successfully, but these errors were encountered: