Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <YuuBrUmiRYoaSmPw@google.com> <20220804142856.306032-1-jrosenth@chromium.org>
 <CAE-0n51UcauP1ojoskV9_2+CFPoJrs08jGHVPCZdSA_oXX1zUw@mail.gmail.com>
 <CAODwPW_mYQ1gcd2Xw5s+7dL2kLcYn-VTxKHUH1iM1V4mn7pT_w@mail.gmail.com> <CAE-0n504q_NeB9e2BYF8P9Scp7TPqsp9KEWu2DO=ZzbYdJfyew@mail.gmail.com>
In-Reply-To: <CAE-0n504q_NeB9e2BYF8P9Scp7TPqsp9KEWu2DO=ZzbYdJfyew@mail.gmail.com>
From:   Julius Werner <jwerner@chromium.org>
Date:   Wed, 10 Aug 2022 15:26:30 -0700
Message-ID: <CAODwPW-LaaD+ptch=R-S4GWoBDjTgWzfvwjyQeBRETZnnosR7g@mail.gmail.com>
Subject: Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Julius Werner <jwerner@chromium.org>,
        Jack Rosenthal <jrosenth@chromium.org>,
        chrome-platform@lists.linux.dev,
        LKML <linux-kernel@vger.kernel.org>,
        Tzung-Bi Shih <tzungbi@kernel.org>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> 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.