Messages in this thread | | | From | Julius Werner <> | Date | Wed, 10 Aug 2022 15:26:30 -0700 | Subject | Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver |
| |
> Alright cool. The bug says 'vbnv' so I was guessing 'nv' meant > non-volatile.
"vbnv" is non-volatile memory, but this driver doesn't actually access vbnv (which is on SPI flash). This driver is meant to access an in-memory cache of vbnv that was put there by firmware.
> > We really only care about one of these right now and many of them will > > never be relevant to the kernel, but I still thought that while we're > > doing this we might as well provide a generic interface to all of them > > because others may become useful in the future (and then you don't > > have to update the kernel every time you get a new use case for some > > userspace tool wanting to interact with some resident data structure > > from coreboot). > > Exposing more than is necessary in the kernel ABI could get into > problems with backwards compatibility. It also means we have to review > the ABI with the consideration that something may change in the future > and cbmem would be exposing something we don't want exposed, or maybe it > is writeable when it shouldn't be?
I think we have a bit of a different concept of what this is supposed to be. Forget about the vbnv and vboot workbuffer parts for the moment, those are where our overall motivation for doing this comes from but those should not be concerns for the kernel itself. From the kernel's point of view, all we have here is a firmware information structure it already knows about (the coreboot tables) pointing to a list of opaque, firmware-specific memory buffers labeled with an ID, and we want it to expose read and write access to those buffers to userspace to make it easier for firmware-specific userspace helper tools to work with them. Note that we already have userspace tools doing that today anyway, e.g. the `cbmem` utility uses /dev/mem to search for and parse the whole coreboot table itself before then using it to access individual CBMEM buffers. But implementing all the logic to do that in each tool that wants to support anything coreboot-specific separately is cumbersome, so we thought that since the kernel already has this coreboot table parsing support anyway, we might as well export the info to userspace to make this job easier. So really all the kernel does here is expose the address, size and ID values from the coreboot table itself, it doesn't (and isn't supposed to) have any understanding about the pointed to buffer. (We could also leave out the `mem` node from this driver and just let our userspace utilities read the `address` and `size` nodes and then use that info to go through /dev/mem. Giving them a direct `mem` node for the buffer just makes that process a bit easier and cleaner.)
So backwards compatibility should not be a concern (unless we're talking about changes in the coreboot table itself, which we strongly try to avoid and which would be an issue for existing kernel drivers already anyway). Understanding of these buffers' contents is explicitly supposed to be the responsibility of the userspace tool that has an easier time keeping up with frequent firmware data format changes and maintaining long lists of back-translating routines for older structure formats for the specific kind of buffer it looks for if necessary (something I specifically wouldn't want to clutter the kernel with).
In regards to write access, I don't really see a point restricting this since all these buffers are already accessible through /dev/mem anyway. But that should only be needed for very few of these buffers anyway, so if people think it's a concern I think we could also keep a small explicit allowlist for the IDs that can be writable (shouldn't need to be updated frequently) and disallow it for everything else. Also, of course there's still the normal file system access permissions that makes these only writable for root and users specifically granted access by root. (Actually, Jack, that reminds me... having the `mem` nodes world-readable is maybe not a good idea, there's usually not anything specifically secret in there, but since /dev/mem also isn't world-readable I think this one probably shouldn't either. I'd suggest changing the initial umask to 0660 or 0600.)
> Furthermore, if the ABI that the kernel can expose already exists then > we're better off because there may already be userspace programs written > for that ABI with lessons learned and bugs ironed out. In this case, I > was hoping that it was an nvmem, in which case we could tie it into the > nvmem framework and piggyback on the nvmem APIs. It also helps to expose > a more generic ABI to userspace instead of developing a bespoke solution > requiring boutique software. Of course sometimes things can't be so > generic so code has to be written.
Yeah but this isn't anything that can be genericized, this is specifically meant for boutique software to maintain its internal data structures. vbnv and the vb2_shared_data structure in the workbuffer are supposed to be completely internal to vboot and not interpreted by any code other than vboot itself -- even coreboot never accesses them directly (only by linking in vboot functions and calling those). We explicitly don't want to deal with having to sync data structure format changes to anywhere outside the vboot repository. The problem is just that unlike most software, vboot is a framework that contains both firmware and userspace components, so the latter necessarily need some help from the kernel (as an oblivious forwarder of opaque data) to be able to access the internal data structures passed on from the former.
| |