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

[Gupta, Akshay]

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.