On Saturday 11 December 2021 16:54:21 Armin Wolf wrote:
> The second switch-case has no real purpose:
>
> - for I8K_BIOS_VERSION, val does not represent a return value,
> making the check for error values unnecessary.
> - for I8K_MACHINE_ID, val remains zero, so the error check is
> unnecessary too.
>
> Remove the switch-case and move the calls to copy_to_user()
> into the first switch-case for I8K_BIOS_VERSION/_MACHINE_ID.
> Omit buff[] since data->bios_machineid already contains the string
> with the necessary zero padding through devm_kzalloc().
>
> Tested on a Dell Inspiron 3505.
>
> Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
Reviewed-by: Pali Rohár <pali@xxxxxxxxxx>
> ---
> drivers/hwmon/dell-smm-hwmon.c | 30 +++++++++---------------------
> 1 file changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 5596c211f38d..186d40938036 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -454,7 +454,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
> {
> int val = 0;
> int speed, err;
> - unsigned char buff[16];
> int __user *argp = (int __user *)arg;
>
> if (!argp)
> @@ -468,15 +467,19 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>
> val = (data->bios_version[0] << 16) |
> (data->bios_version[1] << 8) | data->bios_version[2];
> - break;
>
> + if (copy_to_user(argp, &val, sizeof(val)))
> + return -EFAULT;
> +
> + return 0;
> case I8K_MACHINE_ID:
> if (restricted && !capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> - strscpy_pad(buff, data->bios_machineid, sizeof(buff));
> - break;
> + if (copy_to_user(argp, data->bios_machineid, sizeof(data->bios_machineid)))
> + return -EFAULT;
>
> + return 0;
> case I8K_FN_STATUS:
> val = i8k_get_fn_status();
> break;
> @@ -527,23 +530,8 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
> if (val < 0)
> return val;
>
> - switch (cmd) {
> - case I8K_BIOS_VERSION:
> - if (copy_to_user(argp, &val, 4))
> - return -EFAULT;
> -
> - break;
> - case I8K_MACHINE_ID:
> - if (copy_to_user(argp, buff, 16))
> - return -EFAULT;
> -
> - break;
> - default:
> - if (copy_to_user(argp, &val, sizeof(int)))
> - return -EFAULT;
> -
> - break;
> - }
> + if (copy_to_user(argp, &val, sizeof(int)))
> + return -EFAULT;
>
> return 0;
> }
> --
> 2.30.2
>
