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: <20220929234432.3711480-1-jrosenth@chromium.org>
 <YzaNjlqc0GqmJt68@kroah.com> <YzdqUX/zPvtyCmNm@chromium.org> <YzftVZj+2RCcIDN7@kroah.com>
In-Reply-To: <YzftVZj+2RCcIDN7@kroah.com>
From:   Julius Werner <jwerner@chromium.org>
Date:   Mon, 3 Oct 2022 17:56:35 -0700
Message-ID: <CAODwPW_2bM9d3NzcFDeAYtJeBhWBt10AUOajpNEKmnBOD=Qe2w@mail.gmail.com>
Subject: Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Jack Rosenthal <jrosenth@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>,
        chrome-platform@lists.linux.dev,
        Stephen Boyd <swboyd@chromium.org>,
        Tzung-Bi Shih <tzungbi@kernel.org>,
        Guenter Roeck <groeck@chromium.org>,
        Julius Werner <jwerner@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> No, this is a device, make these device attributes, don't polute sysfs
> with random symlinks from a device into the firmware location, that's
> not ok.
>
> And again, your current code means that tools like udev and libraries
> will not see these attributes at all.  Stick with what every other
> device in the kernel does please, consistancy is good.
>
> > What exactly do we break here by adding symlinks?  udev won't look into
> > /sys/firmware, right?
>
> Exactly.  You want that to work.

I feel like there might be some misunderstanding here... we're not
adding these symlinks for udev. Of course, if someone wanted to match
one of these entries with udev, they would do that by matching the
device entry directly, and there is an `id` device attribute for that.
We're not breaking that use case, and for those tools we totally agree
that the normal device node matching by attribute makes most sense.

But we have a different use case where userspace tools that need to
access one specific ID are frequently called on-demand (e.g. from the
command line or by another process), not only once on device
registration like udev. For that kind of use case, always searching
through every single node to find the right ID is cumbersome and
time-wasting. We're just trying to create a convenience symlink to
make that use case easier and faster. The sysfs device framework
itself already has plenty of convenience symlinks like that... e.g. if
I wanted to find all devices on the coreboot bus, without symlinks I'd
have to iterate through everything in /sys/devices/ and compare each
`subsystem` attribute to check if it matches that bus. But for
convenience sysfs automatically creates symlinks in
/sys/bus/coreboot/devices/. Not sure how this case is so different?

If you really don't like the links, do you have an alternative
suggestion how we could allow our tools to find a single ID quickly
without having to iterate through all entries every time? (I guess
using the device name works but it's a bit cumbersome because then the
bus driver has to dig in deep and inspect device-type-specific details
on registration that would normally only be handled by the device
driver.)

> Also, I asked before, but some note about "exposing all of these bios values to userspace is not a security issue at all" would be nice, if only to point at in a few years and say "wow we were naive"...

To clarify this one some more, it's not so much that there can be no
security implications at all from this, but more that these spaces
have always been accessible through the /dev/mem interface already. So
we're not opening any new holes, we're just providing a new Kconfig
that allows exposing a subset of the attack surface that can already
be exposed via /dev/mem in a more controlled way. Of course users
should still understand the implications of each of these nodes before
they're granting access permissions to it, that's why we're creating
them with umask 0600.