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

["Gupta, Akshay" <Akshay.Gupta@amd.com>]

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.