Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

2020-06-30 Thread Julius Werner
> Ok. Regardless of the concern of the physical address is there any usage
> of this attribute by userspace? The description makes it sound like it's
> a pure debug feature, which implies that it should be in debugfs and not
> in sysfs.

I'll leave that up to Patrick. I doubt we'd want to create a whole
separate debugfs hierarchy just for this. Like I said you can just
read it out of the log too, this would just make it a little bit more
convenient. It's not like it would be the only informational attribute
in sysfs...


Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

2020-06-26 Thread Stephen Boyd
Quoting Julius Werner (2020-06-25 13:51:34)
> > > +What:  /sys/bus/coreboot/devices/.../cbmem_attributes/address
> > > +Date:  Apr 2020
> > > +KernelVersion: 5.6
> > > +Contact:   Patrick Rudolph 
> > > +Description:
> > > +   coreboot device directory can contain a file named
> > > +   cbmem_attributes/address if the device corresponds to a 
> > > CBMEM
> > > +   buffer.
> > > +   The file holds an ASCII representation of the physical 
> > > address
> > > +   of the CBMEM buffer in hex (e.g. 0x8000d000) 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.
> 
> This is memory allocated by firmware and not subject to (k)ASLR, so
> nothing valuable can be leaked here. The same addresses could already
> be parsed out of /sys/firmware/log. Before this interface we usually
> accessed this stuff via /dev/mem (and tools that want to remain
> backwards-compatible will probably want to keep doing that), so having
> a quick shorthand to grab physical addresses can be convenient.

Ok. Regardless of the concern of the physical address is there any usage
of this attribute by userspace? The description makes it sound like it's
a pure debug feature, which implies that it should be in debugfs and not
in sysfs.


Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

2020-06-25 Thread Julius Werner
> > +What:  /sys/bus/coreboot/devices/.../cbmem_attributes/address
> > +Date:  Apr 2020
> > +KernelVersion: 5.6
> > +Contact:   Patrick Rudolph 
> > +Description:
> > +   coreboot device directory can contain a file named
> > +   cbmem_attributes/address if the device corresponds to a 
> > CBMEM
> > +   buffer.
> > +   The file holds an ASCII representation of the physical 
> > address
> > +   of the CBMEM buffer in hex (e.g. 0x8000d000) 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.

This is memory allocated by firmware and not subject to (k)ASLR, so
nothing valuable can be leaked here. The same addresses could already
be parsed out of /sys/firmware/log. Before this interface we usually
accessed this stuff via /dev/mem (and tools that want to remain
backwards-compatible will probably want to keep doing that), so having
a quick shorthand to grab physical addresses can be convenient.


Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

2020-06-25 Thread Stephen Boyd
Quoting patrick.rudo...@9elements.com (2020-04-07 01:29:06)
> From: Patrick Rudolph 
> 
> Make all CBMEM buffers available to userland. This is useful for tools
> that are currently using /dev/mem.
> 
> Make the id, size and address available, as well as the raw table data.
> 
> 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
> 
> 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
> 
> Signed-off-by: Patrick Rudolph 
> ---

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 ..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 
> +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 
> +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 
> +Description:
> +   coreboot device directory can contain a file named
> +   cbmem_attributes/address if the device corresponds to a CBMEM
> +   buffer.
> +   The file holds an ASCII representation of the physical address
> +   of the CBMEM buffer in hex (e.g. 0x8000d000) 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 
> +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 CBMEM
> +   buffer.
> diff --git a/drivers/firmware/google/cbmem-coreboot.c 
> b/drivers/firmware/google/cbmem-coreboot.c
> new file mode 100644
> index ..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 
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 9elements Agency GmbH
> + */
> +
[..]
> +   &bin_attr_data,
> +   NULL
> +};
> +
> +static const struct attribute_group cb_mem_attr_group = {
> +   .name = "cbmem_attributes",
> +   .attrs = cb_mem_attrs,
> +   .bin_attrs = cb_mem_bin_attrs,
> +};
> +
> +static const struct attribute_group *attribute_groups[] = {
> +   &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 = &cdev->dev;
> +   struct cb_priv *priv;
> +
> +   priv = 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;
> +}