| Message ID | 1307393012-32582-2-git-send-email-peterhuewe@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
NAK You should use kstrtou8_from_user() and drop 0xff check as well. Do NOT blindly replace strict_strtoul with kstrtoul. On Mon, Jun 6, 2011 at 11:43 PM, Peter Huewe <peterhuewe@gmx.de> wrote: > - err = strict_strtoul(buf, 0, &user_val); > + err = kstrtoul_from_user(user_buf, count, 0, &user_val); > if (err) > - return -EINVAL; > + return err; > + > if (user_val > 0xff) { > dev_err(dev, "debugfs error input > 0xff\n"); > return -EINVAL;
Am Montag 20 Juni 2011, 15:45:46 schrieb Alexey Dobriyan: > NAK > You should use kstrtou8_from_user() and drop 0xff check as well. > > Do NOT blindly replace strict_strtoul with kstrtoul. Thanks for spotting this - I'll review my other patches as well and redo them. Sorry for any inconvenience. Thanks, Peter
On Mon, Jun 20, 2011 at 09:46:28PM +0200, Peter Huewe wrote: > @@ -923,27 +916,16 @@ static ssize_t ab3550_address_write(struct file *file, > size_t count, loff_t *ppos) > { > struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private; > - char buf[32]; > - int buf_size; > - unsigned long user_address; > + u8 user_address; > int err; > > - /* Get userspace string and assure termination */ > - buf_size = min(count, (sizeof(buf) - 1)); > - if (copy_from_user(buf, user_buf, buf_size)) > - return -EFAULT; > - buf[buf_size] = 0; > - > - err = strict_strtoul(buf, 0, &user_address); > + /* Get userspace string and convert to number */ > + err = kstrtou8_from_user(user_buf, count, 0, &user_address); > if (err) > - return -EINVAL; > - if (user_address > 0xff) { > - dev_err(&ab->i2c_client[0]->dev, > - "debugfs error input > 0xff\n"); > - return -EINVAL; > - } > + return err; > + > ab->debug_address = user_address; > - return buf_size; > + return count; You don't need temporary variable and should write straight to final location, because kstrto* functions will never write to result unless it was converted successfully.
Am Montag 20 Juni 2011, 21:50:38 schrieb Alexey Dobriyan: > On Mon, Jun 20, 2011 at 09:46:28PM +0200, Peter Huewe wrote: > > - char buf[32]; > > - int buf_size; > > - unsigned long user_address; > > + u8 user_address; > > + /* Get userspace string and convert to number */ > > + err = kstrtou8_from_user(user_buf, count, 0, &user_address); ... > > > > ab->debug_address = user_address; > > You don't need temporary variable and should write straight to final > location, because kstrto* functions will never write to result unless it > was converted successfully. Alexey thank you very much for your review, hints and most of all patience ;) The code really gets cleaner and cleaner. While changing the code (once again ;) and looking at your remarks I also saw that ab3550->debug_address and ->debug_bank are always casted to u8. Do you think I could also change the two fields of the struct ab3550 (only used in this file) to u8 in this patch, too? This way I could get rid of the u8* cast which is now needed in this case, if I take you last remark into account. > err = kstrtou8_from_user(user_buf, count, 0, (u8 *)&ab->debug_address); And also clean the code from all the other unnecessary u8 casts. What do you think? Or split it up into two seperate patches? Thanks, Peter
diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c index 64748e4..64bdeeb 100644 --- a/drivers/mfd/ab8500-debugfs.c +++ b/drivers/mfd/ab8500-debugfs.c @@ -419,20 +419,13 @@ static ssize_t ab8500_bank_write(struct file *file, size_t count, loff_t *ppos) { struct device *dev = ((struct seq_file *)(file->private_data))->private; - char buf[32]; - int buf_size; unsigned long user_bank; int err; /* Get userspace string and assure termination */ - buf_size = min(count, (sizeof(buf) - 1)); - if (copy_from_user(buf, user_buf, buf_size)) - return -EFAULT; - buf[buf_size] = 0; - - err = strict_strtoul(buf, 0, &user_bank); + err = kstrtoul_from_user(user_buf, count, 0, &user_bank); if (err) - return -EINVAL; + return err; if (user_bank >= AB8500_NUM_BANKS) { dev_err(dev, "debugfs error input > number of banks\n"); @@ -441,7 +434,7 @@ static ssize_t ab8500_bank_write(struct file *file, debug_bank = user_bank; - return buf_size; + return count; } static int ab8500_address_print(struct seq_file *s, void *p) @@ -459,26 +452,20 @@ static ssize_t ab8500_address_write(struct file *file, size_t count, loff_t *ppos) { struct device *dev = ((struct seq_file *)(file->private_data))->private; - char buf[32]; - int buf_size; unsigned long user_address; int err; /* Get userspace string and assure termination */ - buf_size = min(count, (sizeof(buf) - 1)); - if (copy_from_user(buf, user_buf, buf_size)) - return -EFAULT; - buf[buf_size] = 0; - - err = strict_strtoul(buf, 0, &user_address); + err = kstrtoul_from_user(user_buf, count, 0, &user_address); if (err) - return -EINVAL; + return err; + if (user_address > 0xff) { dev_err(dev, "debugfs error input > 0xff\n"); return -EINVAL; } debug_address = user_address; - return buf_size; + return count; } static int ab8500_val_print(struct seq_file *s, void *p) @@ -509,20 +496,14 @@ static ssize_t ab8500_val_write(struct file *file, size_t count, loff_t *ppos) { struct device *dev = ((struct seq_file *)(file->private_data))->private; - char buf[32]; - int buf_size; unsigned long user_val; int err; /* Get userspace string and assure termination */ - buf_size = min(count, (sizeof(buf)-1)); - if (copy_from_user(buf, user_buf, buf_size)) - return -EFAULT; - buf[buf_size] = 0; - - err = strict_strtoul(buf, 0, &user_val); + err = kstrtoul_from_user(user_buf, count, 0, &user_val); if (err) - return -EINVAL; + return err; + if (user_val > 0xff) { dev_err(dev, "debugfs error input > 0xff\n"); return -EINVAL; @@ -534,7 +515,7 @@ static ssize_t ab8500_val_write(struct file *file, return -EINVAL; } - return buf_size; + return count; } static const struct file_operations ab8500_bank_fops = {
This patch replaces the code for getting an unsigned long from a userspace buffer by a simple call to kstroul_from_user. This makes it easier to read and less error prone. Kernel Version: v3.0-rc2 Signed-off-by: Peter Huewe <peterhuewe@gmx.de> --- drivers/mfd/ab8500-debugfs.c | 41 +++++++++++------------------------------ 1 files changed, 11 insertions(+), 30 deletions(-)