On Monday 29 November 2021 21:57:40 Armin Wolf wrote:
> Am 23.11.21 um 17:13 schrieb Pali Rohár:
>
> > On Saturday 20 November 2021 18:03:18 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->machineid already contains the string
> > s/->machineid/->bios_machineid/
> >
> > > with the necessary zero padding.
> > data is allocated by devm_kzalloc() so data->bios_machineid is really
> > zero padded.
> >
> > > Tested on a Dell Inspiron 3505.
> > >
> > > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > > ---
> > > 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..b5d1703faa62 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, 4))
> > > + 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, 16))
> > What about usage of sizeof(data->bios_machineid) instead of hardcoded
> > constant 16? And maybe same for constant 4?
>
> For the string yes, but maybe i should change the int to an u32?
I do not know if changing int to u32 should be done or not...
> > > + 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
> > >
