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
