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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ