linux-kernel - Re: [PATCH 2/2] drivers/misc: add transfer ioctl for HPS

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]

Date:   Fri, 28 Jan 2022 18:42:08 +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

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 HPS.

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
:)

>
> thanks,
>
> greg k-h


- Sami