| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 11 Apr 2019 16:31:36 -0600
From: Nick Crews <ncrews@...omium.org>
To: Enric Balletbo Serra <eballetbo@...il.com>
Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Benson Leung <bleung@...omium.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Duncan Laurie <dlaurie@...omium.org>,
Daniel Erat <derat@...gle.com>,
Guenter Roeck <groeck@...gle.com>,
Dmitry Torokhov <dtor@...gle.com>,
Simon Glass <sjg@...omium.org>, alevkoy@...omium.org,
keithshort@...omium.org
Subject: Re: [PATCH v3] platform/chrome: wilco_ec: Add h1_gpio status to debugfs
Thanks for the comments Enric! I'll resend in a day or two.
On Thu, Apr 11, 2019 at 3:43 PM Enric Balletbo Serra
<eballetbo@...il.com> wrote:
>
> Hi Nick,
>
> Some comments below ...
>
> Missatge de Nick Crews <ncrews@...omium.org> del dia dj., 11 d’abr.
> 2019 a les 0:09:
> >
> > As part of Chrome OS's FAFT (Fully Automated Firmware Testing)
> > tests, we need to ensure that the H1 chip is properly setting
> > some GPIO lines. The h1_gpio attribute exposes the state
> > of the lines:
> > - ENTRY_TO_FACT_MODE in BIT(0)
> > - SPI_CHROME_SEL in BIT(1)
> >
> > There are two reasons that I am exposing this in debugfs,
> > and not as a GPIO:
> > 1. This is only useful for testing, so end users shouldn't ever
> > care about this. In fact, if it passes the tests, then the value of
> > h1_gpio will always be 2, so it would be really uninteresting for users.
> > 2. This GPIO is not connected to, controlled by, or really even related
> > to the AP. The GPIO runs between the EC and the H1 security chip.
> >
> > Changes in v3:
> > - Fix documentation to correspond with formatting change in v2.
> > Changes in v2:
> > - Zero out the unused fields in the request.
> > - Format result as "%02x\n" instead of as a decimal.
>
> I don't really mind, but wouldn't be more clear prefix with 0x (0x%02x)?
I figured this would be easier for a program to parse, but you're right it
would be more clear. I can change it.
>
> >
> > Signed-off-by: Nick Crews <ncrews@...omium.org>
> > ---
> > drivers/platform/chrome/wilco_ec/debugfs.c | 64 +++++++++++++++++++++-
>
> ABI documentation missing.
Whoops, will do. The Documentation is out of date with the raw
attribute as well,
so I'll fix that too.
>
> > 1 file changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c
> > index 17c4c9068aaf..1b93243c8954 100644
> > --- a/drivers/platform/chrome/wilco_ec/debugfs.c
> > +++ b/drivers/platform/chrome/wilco_ec/debugfs.c
> > @@ -4,7 +4,7 @@
> > *
> > * Copyright 2019 Google LLC
> > *
> > - * There is only one attribute used for debugging, called raw.
> > + * The raw attribute is very useful for EC debugging.
> > * You can write a hexadecimal sentence to raw, and that series of bytes
> > * will be sent to the EC. Then, you can read the bytes of response
> > * by reading from raw.
> > @@ -30,6 +30,16 @@
> > * Note that the first 32 bytes of the received MBOX[] will be printed,
> > * even if some of the data is junk. It is up to you to know how many of
> > * the first bytes of data are the actual response.
> > + *
> > + * There is also another debugfs attribute, called h1_gpio.
> > + * As part of Chrome OS's FAFT (Fully Automated Firmware Testing)
> > + * tests, we need to ensure that the H1 chip is properly setting
> > + * some GPIO lines. The h1_gpio attribute exposes the state
> > + * of the lines:
> > + * - ENTRY_TO_FACT_MODE in BIT(0)
> > + * - SPI_CHROME_SEL in BIT(1)
> > +
> > + * Output will formatted with "%02x\n".
> > */
> >
> > #include <linux/ctype.h>
> > @@ -189,6 +199,56 @@ static const struct file_operations fops_raw = {
> > .llseek = no_llseek,
> > };
> >
> > +#define CMD_KB_CHROME 0x88
> > +#define SUB_CMD_H1_GPIO 0x0A
> > +
> > +struct h1_gpio_status_request {
> > + u8 cmd; /* Always CMD_KB_CHROME */
> > + u8 reserved;
> > + u8 sub_cmd; /* Always SUB_CMD_H1_GPIO */
> > +} __packed;
> > +
> > +struct hi_gpio_status_response {
> > + u8 status; /* 0 if allowed */
> > + u8 val; /* BIT(0)=ENTRY_TO_FACT_MODE, BIT(1)=SPI_CHROME_SEL */
> > +} __packed;
> > +
> > +static ssize_t h1_gpio_read(struct file *file, char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct h1_gpio_status_request rq;
> > + struct hi_gpio_status_response rs;
> > + struct wilco_ec_message msg;
> > + char buf[4];
> > + int ret;
> > +
> > + memset(&rq, 0, sizeof(rq));
> > + rq.cmd = CMD_KB_CHROME;
> > + rq.sub_cmd = SUB_CMD_H1_GPIO;
> > +
> > + memset(&msg, 0, sizeof(msg));
> > + msg.type = WILCO_EC_MSG_LEGACY;
> > + msg.request_data = &rq;
> > + msg.request_size = sizeof(rq);
> > + msg.response_data = &rs;
> > + msg.response_size = sizeof(rs);
> > + ret = wilco_ec_mailbox(debug_info->ec, &msg);
> > + if (ret < 0)
> > + return ret;
> > + if (rs.status)
> > + return -EIO;
> > +
> > + sprintf(buf, "%02x\n", rs.val);
> > +
> > + return simple_read_from_buffer(user_buf, count, ppos, buf, sizeof(buf));
> > +}
> > +
> > +static const struct file_operations fops_h1_gpio = {
> > + .owner = THIS_MODULE,
> > + .read = h1_gpio_read,
> > + .llseek = no_llseek,
> > +};
> > +
>
> I think that I'd use the DEFINE_DEBUGFS_ATTRIBUTE here.
Will do.
>
> > /**
> > * wilco_ec_debugfs_probe() - Create the debugfs node
> > * @pdev: The platform device, probably created in core.c
> > @@ -210,6 +270,8 @@ static int wilco_ec_debugfs_probe(struct platform_device *pdev)
> > if (!debug_info->dir)
> > return 0;
> > debugfs_create_file("raw", 0644, debug_info->dir, NULL, &fops_raw);
> > + debugfs_create_file("h1_gpio", 0444, debug_info->dir, NULL,
> > + &fops_h1_gpio);
> >
> > return 0;
> > }
> > --
> > 2.20.1
> >
>
> Thanks,
> Enric
Powered by blists - more mailing lists