Re: [PATCH v2 2/4] char: rpmb: provide a user space interface

[Arnd Bergmann]

On Tue, Apr 5, 2022 at 11:37 AM Alex Bennée <alex.bennee@xxxxxxxxxx> wrote:

> +static int rpmb_open(struct inode *inode, struct file *fp)
> +{
> +       struct rpmb_dev *rdev;
> +
> +       rdev = 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 = 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 = {
> +               .api_version = 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 = {
> +       .open           = rpmb_open,
> +       .release        = rpmb_release,
> +       .unlocked_ioctl = 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 = 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 request.
> + * @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 reasonable
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 between
that and the actual pointer value.

Whatever you do though, don't use misaligned pointers or implicit padding in 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 the
members so each pointer is naturally aligned.

       Arnd