On Wed, Mar 3, 2021 at 2:54 PM Alex Bennée <alex.bennee@xxxxxxxxxx> wrote:
>
> + /* the rpmb is single open! */
> + if (test_and_set_bit(RPMB_DEV_OPEN, &rdev->status))
> + return -EBUSY;
open counters on device nodes are fundamentally broken, because
they do not stop you from using dup() or sharing the file descriptor
across a fork. Just remove this.
> +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;
> +}
Similarly, API versions are fundamentally flawed, as the kernel requires
us to keep compatibility with existing user space. Remove this as well.
> +static long rpmb_ioctl_cap_cmd(struct rpmb_dev *rdev,
> + struct rpmb_ioc_cap_cmd __user *ptr)
> +{
> + struct rpmb_ioc_cap_cmd cap;
Better do a memset() here to ensure this does not leak kernel
stack data to user space.
> +static const struct file_operations rpmb_fops = {
> + .open = rpmb_open,
> + .release = rpmb_release,
> + .unlocked_ioctl = rpmb_ioctl,
> + .owner = THIS_MODULE,
> + .llseek = noop_llseek,
> +};
Add
.compat_ioctl = compat_ptr_ioctl
to make it work for 32-bit user space on 64-bit kernels.
> @@ -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; }
I don't think it's necessary to make the user interface optional,
I'd just always provide these.
>
> +#define RPMB_API_VERSION 0x80000001
Remove this
> + */
> +struct rpmb_ioc_ver_cmd {
> + __u32 api_version;
> +} __packed;
And this
> +
> +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;
> +} __attribute__((packed));
> +
Remove the packed attribute, it does not change the structure layout but
just makes it less efficient to access on architectures that turn unaligned
loads and stores into byte accesses.
> +/**
> + * struct rpmb_ioc_blocks_cmd - read/write blocks to/from RPMB
> + *
> + * @keyid: key_serial_t of key to use
> + * @addr: index into device (units of 256B blocks)
> + * @count: number of 256B blocks
> + * @data: pointer to data to write/read
> + */
> +struct rpmb_ioc_blocks_cmd {
> + __s32 key; /* key_serial_t */
> + __u32 addr;
> + __u32 count;
> + __u8 __user *data;
> +} __attribute__((packed));
ioctl structures should generally not have pointers in them. If this can be done
one block at a time, you can have the 256 bytes as part of the structure.
This probably needs a redesign anyway based on Tomas' feedback though.
If you end up needing a pointer, use a __u64 member with
u64_to_user_ptr() as described in Documentation/driver-api/ioctl.rst
> +#define RPMB_IOC_VER_CMD _IOR(0xB8, 80, struct rpmb_ioc_ver_cmd)
> +#define RPMB_IOC_CAP_CMD _IOR(0xB8, 81, struct rpmb_ioc_cap_cmd)
> +#define RPMB_IOC_PKEY_CMD _IOW(0xB8, 82, key_serial_t)
> +#define RPMB_IOC_COUNTER_CMD _IOR(0xB8, 84, int)
> +#define RPMB_IOC_WBLOCKS_CMD _IOW(0xB8, 85, struct rpmb_ioc_blocks_cmd)
> +#define RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct rpmb_ioc_blocks_cmd)
The last one should be _IOWR(), not _IOR(), since you write the metadata and
read the data.
Arnd
