Re: [PATCH v2 5/8] fwctl: FWCTL_RPC to execute a Remote Procedure Call to device firmware

Leon Romanovsky
2024-07-30T08:00:44Z

On Mon, Jun 24, 2024 at 07:47:29PM -0300, Jason Gunthorpe 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@nvidia.com>
> ---
> 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,
> };
> 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));
>
> 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);

<...>

> + out_len = cmd->out_len;
> + void *outbuf __free(kvfree_errptr) = fwctl->ops->fw_rpc(
> + ucmd->uctx, cmd->scope, inbuf, cmd->in_len, &out_len);

I was under impression that declaration of variables in C should be at the beginning
of block. Was it changed for the kernel?

Thanks