Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20200407082923.2001556-2-patrick.rudolph@9elements.com>
References: <20200407082923.2001556-1-patrick.rudolph@9elements.com> <20200407082923.2001556-2-patrick.rudolph@9elements.com>
Subject: Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     coreboot@coreboot.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alexios Zavras <alexios.zavras@intel.com>,
        Allison Randal <allison@lohutok.net>,
        Julius Werner <jwerner@chromium.org>,
        Samuel Holland <samuel@sholland.org>
To:     linux-kernel@vger.kernel.org, patrick.rudolph@9elements.com
Date:   Thu, 25 Jun 2020 00:05:38 -0700
Message-ID: <159306873839.62212.9311861115757727633@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting patrick.rudolph@9elements.com (2020-04-07 01:29:06)
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>=20
> Make all CBMEM buffers available to userland. This is useful for tools
> that are currently using /dev/mem.
>=20
> Make the id, size and address available, as well as the raw table data.
>=20
> Tools can easily scan the right CBMEM buffer by reading
> /sys/bus/coreboot/drivers/cbmem/coreboot*/cbmem_attributes/id
> or
> /sys/bus/coreboot/devices/coreboot*/cbmem_attributes/id
>=20
> The binary table data can then be read from
> /sys/bus/coreboot/drivers/cbmem/coreboot*/cbmem_attributes/data
> or
> /sys/bus/coreboot/devices/coreboot*/cbmem_attributes/data
>=20
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---

Sorry, this fell off my radar. Looks close though so please resend.

> diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot b/Documentation/=
ABI/stable/sysfs-bus-coreboot
> new file mode 100644
> index 000000000000..6055906f41f2
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-bus-coreboot
> @@ -0,0 +1,44 @@
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/id
> +Date:          Apr 2020
> +KernelVersion: 5.6
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/id if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds an ASCII representation of the CBMEM ID in=
 hex
> +               (e.g. 0xdeadbeef).
> +
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/size
> +Date:          Apr 2020
> +KernelVersion: 5.6
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/size if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds an representation as decimal number of the
> +               CBMEM buffer size (e.g. 64).
> +
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/address
> +Date:          Apr 2020
> +KernelVersion: 5.6
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/address if the device corresponds to a C=
BMEM
> +               buffer.
> +               The file holds an ASCII representation of the physical ad=
dress
> +               of the CBMEM buffer in hex (e.g. 0x000000008000d000) and =
should
> +               be used for debugging only.

If this is for debugging purposes only perhaps it should go into
debugfs. We try to not leak information about physical addresses to
userspace and this would let an attacker understand where memory may be.
That's not ideal and should be avoided.

> +
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/data
> +Date:          Apr 2020
> +KernelVersion: 5.6
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/data if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds a read-only binary representation of the C=
BMEM
> +               buffer.
> diff --git a/drivers/firmware/google/cbmem-coreboot.c b/drivers/firmware/=
google/cbmem-coreboot.c
> new file mode 100644
> index 000000000000..f76704a6eec7
> --- /dev/null
> +++ b/drivers/firmware/google/cbmem-coreboot.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * cbmem-coreboot.c
> + *
> + * Exports CBMEM as attributes in sysfs.
> + *
> + * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 9elements Agency GmbH
> + */
> +
[..]
> +       &bin_attr_data,
> +       NULL
> +};
> +
> +static const struct attribute_group cb_mem_attr_group =3D {
> +       .name =3D "cbmem_attributes",
> +       .attrs =3D cb_mem_attrs,
> +       .bin_attrs =3D cb_mem_bin_attrs,
> +};
> +
> +static const struct attribute_group *attribute_groups[] =3D {
> +       &cb_mem_attr_group,
> +       NULL,

Nitpick: Drop the comma on sentinel so nothing can come after lest a
compile error happens.

> +};
> +
> +static int cbmem_probe(struct coreboot_device *cdev)
> +{
> +       struct device *dev =3D &cdev->dev;
> +       struct cb_priv *priv;
> +
> +       priv =3D devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> +
> +       dev_set_drvdata(dev, priv);

Agreed, avoid the memcpy().

> +
> +       return 0;
> +}