Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220405093759.1126835-1-alex.bennee@linaro.org> <20220405093759.1126835-3-alex.bennee@linaro.org>
In-Reply-To: <20220405093759.1126835-3-alex.bennee@linaro.org>
From:   Arnd Bergmann <arnd@linaro.org>
Date:   Thu, 16 Jun 2022 21:21:56 +0200
Message-ID: <CAK8P3a1xC5jVT9dE7GTtMhE5siFzOkgb5uBEC-ZCt5GPW09u+g@mail.gmail.com>
Subject: Re: [PATCH v2 2/4] char: rpmb: provide a user space interface
To:     =?UTF-8?B?QWxleCBCZW5uw6ll?= <alex.bennee@linaro.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Maxim Uvarov <maxim.uvarov@linaro.org>,
        Joakim Bech <joakim.bech@linaro.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        Ruchika Gupta <ruchika.gupta@linaro.org>,
        "Winkler, Tomas" <tomas.winkler@intel.com>,
        "Huang, Yang" <yang.huang@intel.com>,
        "Zhu, Bing" <bing.zhu@intel.com>, Matti.Moell@opensynergy.com,
        hmo@opensynergy.com, linux-mmc <linux-mmc@vger.kernel.org>,
        linux-scsi <linux-scsi@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Arnd Bergmann <arnd.bergmann@linaro.org>,
        Alexander Usyskin <alexander.usyskin@intel.com>,
        Avri Altman <avri.altman@sandisk.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Apr 5, 2022 at 11:37 AM Alex Benn=C3=A9e <alex.bennee@linaro.org> w=
rote:

> +static int rpmb_open(struct inode *inode, struct file *fp)
> +{
> +       struct rpmb_dev *rdev;
> +
> +       rdev =3D container_of(inode->i_cdev, struct rpmb_dev, cdev);
> +       if (!rdev)
> +               return -ENODEV;
> +
> +       /* the rpmb is single open! */
> +       if (test_and_set_bit(RPMB_DEV_OPEN, &rdev->status))
> +               return -EBUSY;
> +
> +       mutex_lock(&rdev->lock);
> +
> +       fp->private_data =3D rdev;
> +
> +       mutex_unlock(&rdev->lock);

The mutex here doesn't really do anything, as the file structure is not
reachable from any other context.

I'm not sure the single-open protection gains you anything either,
as it does not prevent the file structure to be shared between
processes.

> +static long rpmb_ioctl_ver_cmd(struct rpmb_dev *rdev,
> +                              struct rpmb_ioc_ver_cmd __user *ptr)
> +{
> +       struct rpmb_ioc_ver_cmd ver =3D {
> +               .api_version =3D RPMB_API_VERSION,
> +       };
> +
> +       return copy_to_user(ptr, &ver, sizeof(ver)) ? -EFAULT : 0;
> +}

API version ioctls are generally a bad idea, I think this should be skipped=
.
The way to handle incompatible API changes is to introduce new ioctl
commands.

> +static const struct file_operations rpmb_fops =3D {
> +       .open           =3D rpmb_open,
> +       .release        =3D rpmb_release,
> +       .unlocked_ioctl =3D rpmb_ioctl,


> diff --git a/drivers/rpmb/rpmb-cdev.h b/drivers/rpmb/rpmb-cdev.h
> new file mode 100644
> index 000000000000..e59ff0c05e9d
> --- /dev/null
> +++ b/drivers/rpmb/rpmb-cdev.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
> +/*
> + * Copyright (C) 2015-2018 Intel Corp. All rights reserved
> + */
> +#ifdef CONFIG_RPMB_INTF_DEV
> +int __init rpmb_cdev_init(void);
> +void __exit rpmb_cdev_exit(void);
> +void rpmb_cdev_prepare(struct rpmb_dev *rdev);
> +void rpmb_cdev_add(struct rpmb_dev *rdev);
> +void rpmb_cdev_del(struct rpmb_dev *rdev);
> +#else
> +static inline int __init rpmb_cdev_init(void) { return 0; }
> +static inline void __exit rpmb_cdev_exit(void) {}
> +static inline void rpmb_cdev_prepare(struct rpmb_dev *rdev) {}
> +static inline void rpmb_cdev_add(struct rpmb_dev *rdev) {}
> +static inline void rpmb_cdev_del(struct rpmb_dev *rdev) {}
> +#endif /* CONFIG_RPMB_INTF_DEV */

What is the purpose of the fallback? Would you ever build the code without
the interface?

> +/**
> + * struct rpmb_ioc_ver_cmd - rpmb api version
> + *
> + * @api_version: rpmb API version.
> + */
> +struct rpmb_ioc_ver_cmd {
> +       __u32 api_version;
> +} __packed;

Best remove this completely.

> +enum rpmb_auth_method {
> +       RPMB_HMAC_ALGO_SHA_256 =3D 0,
> +};
> +
> +/**
> + * struct rpmb_ioc_cap_cmd - rpmb capabilities
> + *
> + * @target: rpmb target/region within RPMB partition.
> + * @capacity: storage capacity (in units of 128K)
> + * @block_size: storage data block size (in units of 256B)
> + * @wr_cnt_max: maximal number of block that can be written in a single =
request.
> + * @rd_cnt_max: maximal number of block that can be read in a single req=
uest.
> + * @auth_method: authentication method: currently always HMAC_SHA_256
> + * @reserved: reserved to align to 4 bytes.
> + */
> +struct rpmb_ioc_cap_cmd {
> +       __u16 target;
> +       __u16 capacity;
> +       __u16 block_size;
> +       __u16 wr_cnt_max;
> +       __u16 rd_cnt_max;
> +       __u16 auth_method;
> +       __u16 reserved;
> +} __packed;

I would remove the __packed attribute here. If you want to keep it, then
it needs to have a __aligned() attribute as well.

It seems strange to have an odd number of 16-bit fields in this, maybe
make it two reserved fields to have a multiple of 32-bit.

> + * struct rpmb_ioc_reqresp_cmd - general purpose reqresp
> + *
> + * Most RPMB operations consist of a set of request frames and an
> + * optional response frame. If a response is requested the user must
> + * allocate enough space for the response, otherwise the fields should
> + * be set to 0/NULL.
> + *
> + * It is used for programming the key, reading the counter and writing
> + * blocks to the device. If the frames are malformed they may be
> + * rejected by the underlying driver or the device itself.
> + *
> + * Assuming the transaction succeeds it is still up to user space to
> + * validate the response and check MAC values correspond to the
> + * programmed keys.
> + *
> + * @len: length of write counter request
> + * @request: ptr to device specific request frame
> + * @rlen: length of response frame
> + * @resp: ptr to device specific response frame
> + */
> +struct rpmb_ioc_reqresp_cmd {
> +       __u32 len;
> +       __u8 __user *request;
> +       __u32 rlen;
> +       __u8 __user *response;
> +} __packed;

Try to avoid indirect pointers in ioctl structures, they break
compatibility with
32-bit processes. I think you can do this with a variable-length structure =
that
has the two length fields followed by the actual data. If there is a reason=
able
upper bound for the length, it could also just be a fixed-length buffer.

If you end up having to use a pointer, make it a __u64 value and cast betwe=
en
that and the actual pointer value.

Whatever you do though, don't use misaligned pointers or implicit padding i=
n the
structure. You can have explicit padding by adding an extra __u32 before
a __u64 pointer value, and then remove the __packed attribute, or reorder t=
he
members so each pointer is naturally aligned.

       Arnd