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;
Date:   Thu, 29 Sep 2022 17:57:03 -0600
From:   Jack Rosenthal <jrosenth@chromium.org>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     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>
Subject: Re: [RESEND PATCH v10] firmware: google: Implement cbmem in sysfs
 driver
Message-ID: <YzYwz8puWz+zcaF/@chromium.org>
References: <20220927205551.2017473-1-jrosenth@chromium.org>
 <YzPmKF6LRwXVua3X@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YzPmKF6LRwXVua3X@kroah.com>
Precedence: bulk

On 2022-09-28 at 08:14 +0200, Greg KH wrote:
> Why is this a RESEND?
> 
> I don't remember seeing any of the 10 previous versions, but hey, I
> can't remember much...

I sent the last 10 versions (including just v10) to the mailing lists
without including you.  Realizing I should include you, I resent.  Sorry
about that.

> Why is "coreboot" needed here?

I just did this for more heiarchy, and it's not necessary.  This is just
/sys/firmware/cbmem in the latest patchset.

> Will this work for all coreboot implementations?

Yes, CBMEM has existed in coreboot since 2009, so it'll work with any
non-antique version.

> Raw memory addresses?  Why?

Two reasons:

(1) It's useful information while debugging changes you've made to
coreboot (e.g., if you were adding a new data structure to cbmem).

(2) The "cbmem" command line tool exposes this information, and it would
be nice to entirely replace it's current /dev/mem interface with this
sysfs interface once it's widely available.

> Nothing is being "stored" here, it is being read.

Right, fixed in v11, thanks.

> Why doesn't mmap work?

Coreboot won't guarantee page alignment.  I updated this text in v11 to
mention that.

> What's the security implications of exposing all of this information?

Nothing really.  These are data structures from firmware such as the
nvram, boot log, flash layout, etc.  It's not something that would
contain secret information.

The biggest consideration is that we allow access to memory regions.
Since the coreboot table may be a CBMEM region itself, we just make sure
to copy the entry's size from the coreboot table before it's available
in sysfs so manipulation of the coreboot table won't allow arbitrary
memory access.

> Please use an attribute group so that this all happens automatically and
> you do not have to hand add and remove files.
> 
> That will make this logic much simpler and cleaner.

Thanks, v11 has it =)

> > +/* Corresponds to LB_TAG_CBMEM_ENTRY */
> > +struct lb_cbmem_entry {
> > +	u32 tag;
> > +	u32 size;
> > +
> > +	u64 address;
> > +	u32 entry_size;
> > +	u32 id;
> 
> As these cross the user/kernel boundry shouldn't they be __u32 and
> __u64?  Or is that not how coreboot works?

This data structure isn't exposed to user space in its raw format (we
give access to it via %x-encoded output from the sysfs files).