Re: [PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Mar 3, 2025, at 11:58, Akshay Gupta wrote:

> +static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned 
> long arg)
> +{
> +	int __user *arguser = (int  __user *)arg;
> +	struct apml_message msg = { 0 };
> +	bool read = false;
> +	int ret;
> +
> +	struct sbrmi_data *data = container_of(fp->private_data, struct 
> sbrmi_data,
> +					       sbrmi_misc_dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	/* Copy the structure from user */
> +	if (copy_struct_from_user(&msg, sizeof(msg), arguser,
> +				  sizeof(struct apml_message)))
> +		return -EFAULT;

This is not how ioctl commands work: you need to check the
'cmd' argument, which includes the length of the data.

copy_struct_from_user() makes no sense here since the length
is fixed (for a given command).

> +	switch (msg.cmd) {
> +	case 0 ... 0x999:
> +		/* Mailbox protocol */
> +		ret = rmi_mailbox_xfer(data, &msg);
> +		break;

This looks like you are blindly passing through any command
from userspace, which is generally not the preferred way.

Usually this should be a known set of high-level commands
accepted by the driver.

> +static const struct file_operations sbrmi_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= sbrmi_ioctl,
> +	.compat_ioctl	= sbrmi_ioctl,

Change this to

.compat_ioctl = compat_ptr_ioctl,


> +	data->sbrmi_misc_dev.name	= devm_kasprintf(dev,
> +							 GFP_KERNEL,
> +							 "sbrmi-%x",
> +							 data->dev_static_addr);
> +	data->sbrmi_misc_dev.minor	= MISC_DYNAMIC_MINOR;
> +	data->sbrmi_misc_dev.fops	= &sbrmi_fops;
> +	data->sbrmi_misc_dev.parent	= dev;
> +	data->sbrmi_misc_dev.nodename	= devm_kasprintf(dev,
> +							 GFP_KERNEL,
> +							 "sbrmi-%x",
> +							 data->dev_static_addr);
> +	data->sbrmi_misc_dev.mode	= 0600;
> +
> +	return misc_register(&data->sbrmi_misc_dev);

What is 'dev_static_addr'? Usually you want a miscdevice to 
have a constant name and a static structure definition, not
dynamic allocation.

Are there multiple devices of this type in a given system?

> +struct apml_message {
> +	/* message ids:
> +	 * Mailbox Messages:	0x0 ... 0x999
> +	 */
> +	__u32 cmd;
> +
> +	/*
> +	 * 8 bit data for reg read,
> +	 * 32 bit data in case of mailbox,
> +	 */
> +	union {
> +		__u32 mb_out[2];
> +		__u8 reg_out[8];
> +	} data_out;
> +
> +	/*
> +	 * [0]...[3] mailbox 32bit input
> +	 * [7] read/write functionality
> +	 */
> +	union {
> +		__u32 mb_in[2];
> +		__u8 reg_in[8];
> +	} data_in;
> +} __attribute__((packed));

You normally want to have the in-kernel data aligned. Even
if userspace has it at a misaligned offset, it will still
work without the __packed.

     Arnd