| 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
| ||
|
Message-ID: <CAPuLczsP=VFVYzFjGDj3ysb6C=p4FHMVO3T69P6oN+p0WFQqRw@mail.gmail.com>
Date: Mon, 31 Jan 2022 19:23:24 +1100
From: Sami Kyostila <skyostil@...omium.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>, dtor@...omium.org,
evanbenn@...omium.org, arnd@...db.de
Subject: Re: [PATCH 2/2] drivers/misc: add transfer ioctl for HPS
pe 28. tammik. 2022 klo 20.36 Greg KH (gregkh@...uxfoundation.org) kirjoitti:
>
> On Fri, Jan 28, 2022 at 06:42:08PM +1100, Sami Kyostila wrote:
> > to 27. tammik. 2022 klo 20.41 Greg KH (gregkh@...uxfoundation.org) kirjoitti:
> > >
> > > On Thu, Jan 27, 2022 at 07:35:45PM +1100, Sami Kyöstilä wrote:
> > > > This patch adds an ioctl operation for sending and receiving data from
> > > > the ChromeOS snooping protection sensor (a.k.a., HPS). This allows
> > > > userspace programs to perform a combined read/write I2C transaction
> > > > through a single syscall.
> > > >
> > > > The I2C wire protocol for the device is documented at:
> > > >
> > > > https://chromium.googlesource.com/chromiumos/platform/hps-firmware/+/
> > > > refs/heads/main/docs/host_device_i2c_protocol.md
> > > >
> > > > Signed-off-by: Sami Kyöstilä <skyostil@...omium.org>
> > > > ---
> > > >
> > > > MAINTAINERS | 1 +
> > > > drivers/misc/hps-i2c.c | 81 ++++++++++++++++++++++++++++++++++++++++
> > > > include/uapi/linux/hps.h | 20 ++++++++++
> > > > 3 files changed, 102 insertions(+)
> > > > create mode 100644 include/uapi/linux/hps.h
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 9dea4b8c2ab5..d5fc066fdbc2 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -8803,6 +8803,7 @@ M: Sami Kyöstilä <skyostil@...omium.org>
> > > > R: Evan Benn <evanbenn@...omium.org>
> > > > S: Maintained
> > > > F: drivers/misc/hps-i2c.c
> > > > +F: include/uapi/linux/hps.h
> > > >
> > > > HSI SUBSYSTEM
> > > > M: Sebastian Reichel <sre@...nel.org>
> > > > diff --git a/drivers/misc/hps-i2c.c b/drivers/misc/hps-i2c.c
> > > > index fe9f073b0352..748ead49d678 100644
> > > > --- a/drivers/misc/hps-i2c.c
> > > > +++ b/drivers/misc/hps-i2c.c
> > > > @@ -17,9 +17,11 @@
> > > > #include <linux/i2c.h>
> > > > #include <linux/module.h>
> > > > #include <linux/pm_runtime.h>
> > > > +#include <uapi/linux/hps.h>
> > > >
> > > > #define HPS_ACPI_ID "GOOG0020"
> > > > #define HPS_MAX_DEVICES 1
> > > > +#define HPS_MAX_MSG_SIZE 8192
> > > >
> > > > struct hps_drvdata {
> > > > struct i2c_client *client;
> > > > @@ -60,6 +62,8 @@ static int hps_open(struct inode *inode, struct file *file)
> > > > ret = pm_runtime_get_sync(dev);
> > > > if (ret < 0)
> > > > goto pm_get_fail;
> > > > +
> > > > + file->private_data = hps->client;
> > > > return 0;
> > > >
> > > > pm_get_fail:
> > > > @@ -84,10 +88,87 @@ static int hps_release(struct inode *inode, struct file *file)
> > > > return ret;
> > > > }
> > > >
> > > > +static int hps_do_ioctl_transfer(struct i2c_client *client,
> > > > + struct hps_transfer_ioctl_data *args)
> > > > +{
> > > > + int ret;
> > > > + int nmsg = 0;
> > > > + struct i2c_msg msgs[2] = {
> > > > + {
> > > > + .addr = client->addr,
> > > > + .flags = client->flags,
> > > > + },
> > > > + {
> > > > + .addr = client->addr,
> > > > + .flags = client->flags,
> > > > + },
> > > > + };
> > > > +
> > > > + if (args->isize) {
> > > > + msgs[nmsg].len = args->isize;
> > > > + msgs[nmsg].buf = memdup_user(args->ibuf, args->isize);
> > > > + if (IS_ERR(msgs[nmsg].buf)) {
> > > > + ret = PTR_ERR(msgs[nmsg].buf);
> > > > + goto memdup_fail;
> > > > + }
> > > > + nmsg++;
> > > > + }
> > > > +
> > > > + if (args->osize) {
> > > > + msgs[nmsg].len = args->osize;
> > > > + msgs[nmsg].buf = memdup_user(args->obuf, args->osize);
> > > > + msgs[nmsg].flags |= I2C_M_RD;
> > > > + if (IS_ERR(msgs[nmsg].buf)) {
> > > > + ret = PTR_ERR(msgs[nmsg].buf);
> > > > + goto memdup_fail;
> > > > + }
> > > > + nmsg++;
> > > > + }
> > > > +
> > > > + ret = i2c_transfer(client->adapter, &msgs[0], nmsg);
> > > > + if (ret > 0 && args->osize) {
> > > > + if (copy_to_user(args->obuf, msgs[nmsg - 1].buf, ret))
> > > > + ret = -EFAULT;
> > > > + }
> > >
> > > Why can't you just do normal i2c transfers to/from userspace instead of
> > > requring a custom ioctl?
> >
> > The main reason is security: without this driver, in order to talk to
> > HPS the userspace daemon needs read/write access to the entire I2C
> > controller (e.g., /dev/i2c-0). This means the daemon can also control
> > any other I2C device which happens to be on the same bus. With a
> > separate ioctl we can limit access to just HP
> Then use seccomp for this?
As far as I can tell, seccomp can't do this because it can't read the
device address field[1] which is behind two userspace pointer hops in
the I2C_RDWR ioctl[2].
[1] https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c.h#L74
[2] https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c-dev.h#L51
>
> > As far as I can tell, there's no way to restrict access on a
> > per-device level with normal i2c, but I'd be happy to be proven wrong
> > :)
>
> Selinux rules?
I guess we could add an LSM hook for I2C transfers, but that would
require baking device addresses into the SELinux policy which seems a
little unfortunate.
I think that leaves the options suggested by Arnd (thanks!):
a) Add a generic way to expose device nodes for individual I2C devices
(something like /dev/i2c/by-id/NN?).
b) Make the ioctl interface more fully featured instead of just exposing I2C.
I think I'm leaning toward (a) since it's not yet totally clear what
the right high level abstraction for this type of device is (e.g.,
should it be HID, in which case the protocol should probably become
HID-I2C).
If this sounds okay, then I suggest we merge just the power control
patch from this series (with comments addressed) and I'll follow-up
with splitting out the i2c devices.
> What else is on this bus for the device that you care about?
That's really up to the device manufacturer; it could be anything from
touchpads to cameras or fingerprint scanners.
> thanks,
>
> greg k-h
- Sami
Powered by blists - more mailing lists