On 3/3/2025 9:48 PM, Arnd Bergmann wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
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.
will add check in ioctl for 'cmd'. Thank you.
copy_struct_from_user() makes no sense here since the length
is fixed (for a given command).
will modify it to use copy_from_user and size using the _IOC_SIZE(cmd)
+ 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.
This will be addressed
+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,
sure.
+ 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?
Yes, there can be multiple devices on the basis of number of nodes.
+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
Thank you, will update.
