Re: [PATCH 1/3] lsm: add hook for firmware command validation

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon,  9 Mar 2026 13:15:18 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> From: Chiara Meiohas <cmeiohas@nvidia.com>
> 
> Drivers typically communicate with device firmware either via
> register-based commands (writing parameters into device registers)
> or by passing a command buffer using shared-memory mechanisms.
> 
> This hook targets the command buffer mechanism, which is commonly
> used on modern, complex devices.
> 
> Add the LSM hook fw_validate_cmd. This hook allows inspecting
> firmware command buffers before they are sent to the device.
> The hook receives the command buffer, device, command class, and a
> class-specific id:
>   - class_id (enum fw_cmd_class) allows security modules to
>     differentiate between classes of firmware commands.
>     In this series, class_id distinguishes between commands from the
>     RDMA uverbs interface and from fwctl.
>   - id is a class-specific device identifier. For uverbs, id is the
>     RDMA driver identifier (enum rdma_driver_id). For fwctl, id is the
>     device type (enum fwctl_device_type).
> 
> Signed-off-by: Chiara Meiohas <cmeiohas@nvidia.com>
> Reviewed-by: Maher Sanalla <msanalla@nvidia.com>
> Signed-off-by: Edward Srouji <edwards@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
Hi Leon,

To me this seems sensible, but LSM isn't an area I know that much about.

With that in mind:
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

A few formatting related comments inline.

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 83a646d72f6f8..64786d013207a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -67,6 +67,7 @@ enum fs_value_type;
>  struct watch;
>  struct watch_notification;
>  struct lsm_ctx;
> +struct device;
>  
>  /* Default (no) options for the capable function */
>  #define CAP_OPT_NONE 0x0
> @@ -157,6 +158,21 @@ enum lockdown_reason {
>  	LOCKDOWN_CONFIDENTIALITY_MAX,
>  };
>  
> +/*
Could add the MAX entry and making this /**
The file is a bit inconsistent on that.

> + * enum fw_cmd_class - Class of the firmware command passed to
> + * security_fw_validate_cmd.
> + * This allows security modules to distinguish between different command
> + * classes.
> + *
> + * @FW_CMD_CLASS_UVERBS: Command originated from the RDMA uverbs interface
> + * @FW_CMD_CLASS_FWCTL: Command originated from the fwctl interface
> + */
> +enum fw_cmd_class {
> +	FW_CMD_CLASS_UVERBS,
> +	FW_CMD_CLASS_FWCTL,
> +	FW_CMD_CLASS_MAX,
Nitpick. Drop the trailing comma to make it a tiny bit more obvious if
someone accidentally adds anything after this counting entry.

> +};
> +
>  /*
>   * Data exported by the security modules
>   */
> @@ -575,6 +591,9 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>  int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
>  int security_inode_getsecctx(struct inode *inode, struct lsm_context *cp);
>  int security_locked_down(enum lockdown_reason what);
> +int security_fw_validate_cmd(const void *in, size_t in_len,
> +			     const struct device *dev,
> +			     enum fw_cmd_class class_id, u32 id);
>  int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
>  		      void *val, size_t val_len, u64 id, u64 flags);
>  int security_bdev_alloc(struct block_device *bdev);
> @@ -1589,6 +1608,12 @@ static inline int security_locked_down(enum lockdown_reason what)
>  {
>  	return 0;
>  }
> +static inline int security_fw_validate_cmd(const void *in, size_t in_len,
> +					   const struct device *dev,
> +					   enum fw_cmd_class class_id, u32 id)
> +{
> +	return 0;
> +}
>  static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
>  				    u32 *uctx_len, void *val, size_t val_len,
>  				    u64 id, u64 flags)
> diff --git a/security/security.c b/security/security.c
> index 67af9228c4e94..d05941fe89a48 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5373,6 +5373,32 @@ int security_locked_down(enum lockdown_reason what)
>  }
>  EXPORT_SYMBOL(security_locked_down);
>  
> +/**
> + * security_fw_validate_cmd() - Validate a firmware command
> + * @in: pointer to the firmware command input buffer
> + * @in_len: length of the firmware command input buffer
> + * @dev: device associated with the command
> + * @class_id: class of the firmware command
> + * @id: device identifier, specific to the command @class_id
> + *
> + * Check permissions before sending a firmware command generated by
> + * userspace to the device.
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_fw_validate_cmd(const void *in, size_t in_len,
> +			     const struct device *dev,
> +			     enum fw_cmd_class class_id,
> +			     u32 id)

I'd follow the wrapping you have in the header and have id on the line
above.

> +{
> +	if (class_id >= FW_CMD_CLASS_MAX)
> +		return -EINVAL;
> +
> +	return call_int_hook(fw_validate_cmd, in, in_len,
> +			     dev, class_id, id);

Fits on one line < 80 chars.

> +}
> +EXPORT_SYMBOL_GPL(security_fw_validate_cmd);
> +
>  /**
>   * security_bdev_alloc() - Allocate a block device LSM blob
>   * @bdev: block device
>