On Mon, 24 Jun 2024 19:47:29 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> Add the FWCTL_RPC ioctl which allows a request/response RPC call to device
> firmware. Drivers implementing this call must follow the security
> guidelines under Documentation/userspace-api/fwctl.rst
>
> The core code provides some memory management helpers to get the messages
> copied from and back to userspace. The driver is responsible for
> allocating the output message memory and delivering the message to the
> device.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
A few minor things inline.
> ---
> drivers/fwctl/main.c | 62 +++++++++++++++++++++++++++++++++++
> include/linux/fwctl.h | 5 +++
> include/uapi/fwctl/fwctl.h | 66 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 133 insertions(+)
>
> diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c
> index f1dec0b590aee4..9506b993a1a56d 100644
> --- a/drivers/fwctl/main.c
> +++ b/drivers/fwctl/main.c
> @@ -8,16 +8,20 @@
> #include <linux/slab.h>
> #include <linux/container_of.h>
> #include <linux/fs.h>
> +#include <linux/sizes.h>
>
> #include <uapi/fwctl/fwctl.h>
>
> enum {
> FWCTL_MAX_DEVICES = 256,
> + MAX_RPC_LEN = SZ_2M,
> };
In what way is that usefully handled as an enum?
I'd just use #defines
> static dev_t fwctl_dev;
> static DEFINE_IDA(fwctl_ida);
> +static unsigned long fwctl_tainted;
>
> DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
> +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
kvfree define free already defined as this since 6.9
>
> struct fwctl_ucmd {
> struct fwctl_uctx *uctx;
> @@ -75,9 +79,66 @@ static int fwctl_cmd_info(struct fwctl_ucmd *ucmd)
> return ucmd_respond(ucmd, sizeof(*cmd));
> }
>
> +static int fwctl_cmd_rpc(struct fwctl_ucmd *ucmd)
> +{
> + struct fwctl_device *fwctl = ucmd->uctx->fwctl;
> + struct fwctl_rpc *cmd = ucmd->cmd;
> + size_t out_len;
> +
> + if (cmd->in_len > MAX_RPC_LEN || cmd->out_len > MAX_RPC_LEN)
> + return -EMSGSIZE;
> +
> + switch (cmd->scope) {
> + case FWCTL_RPC_CONFIGURATION:
> + case FWCTL_RPC_DEBUG_READ_ONLY:
> + break;
> +
> + case FWCTL_RPC_DEBUG_WRITE_FULL:
> + if (!capable(CAP_SYS_RAWIO))
> + return -EPERM;
> + fallthrough;
> + case FWCTL_RPC_DEBUG_WRITE:
> + if (!test_and_set_bit(0, &fwctl_tainted)) {
> + dev_warn(
> + &fwctl->dev,
> + "%s(%d): has requested full access to the physical device device",
> + current->comm, task_pid_nr(current));
> + add_taint(TAINT_FWCTL, LOCKDEP_STILL_OK);
> + }
> + break;
> + default:
> + return -EOPNOTSUPP;
> + };
> +
> + void *inbuf __free(kvfree) =
> + kvzalloc(cmd->in_len, GFP_KERNEL | GFP_KERNEL_ACCOUNT);
As before
#define GFP_KERNEL_ACCOUNT (GFP_KERNEL | __GFP_ACCOUNT)
so don't need both.
> + if (!inbuf)
> + return -ENOMEM;
> + if (copy_from_user(inbuf, u64_to_user_ptr(cmd->in), cmd->in_len))
> + return -EFAULT;
> +
> + out_len = cmd->out_len;
> + void *outbuf __free(kvfree_errptr) = fwctl->ops->fw_rpc(
> + ucmd->uctx, cmd->scope, inbuf, cmd->in_len, &out_len);
> + if (IS_ERR(outbuf))
> + return PTR_ERR(outbuf);
> + if (outbuf == inbuf) {
> + /* The driver can re-use inbuf as outbuf */
> + inbuf = NULL;
I wish no_free_ptr() didn't have __must_check. Can do something ugly
like
outbuf = no_free_ptr(inbuf);
probably but maybe just setting it NULL is simpler.
> + }
> +
> + if (copy_to_user(u64_to_user_ptr(cmd->out), outbuf,
> + min(cmd->out_len, out_len)))
> + return -EFAULT;
> +
> + cmd->out_len = out_len;
> + return ucmd_respond(ucmd, sizeof(*cmd));
> +}
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 39db9f09f8068e..8bde0d4416fd55 100644
> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -67,4 +67,70 @@ struct fwctl_info {
> };
> #define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO)
>
> +/**
> + * enum fwctl_rpc_scope - Scope of access for the RPC
> + */
> +enum fwctl_rpc_scope {
...
> + /**
> + * @FWCTL_RPC_DEBUG_READ_ONLY: Read only access to debug information
> + *
> + * Readable debug information. Debug information is compatible with
> + * kernel lockdown, and does not disclose any sensitive information. For
> + * instance exposing any encryption secrets from this information is
> + * forbidden.
> + */
> + FWCTL_RPC_DEBUG_READ_ONLY = 1,
> + /**
> + * @FWCTL_RPC_DEBUG_WRITE: Writable access to lockdown compatible debug information
Write access
probably rather than writeable.
> + *
> + * Allows write access to data in the device which may leave a fully
> + * supported state. This is intended to permit intensive and possibly
> + * invasive debugging. This scope will taint the kernel.
> + */
> +};
