| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <B567DBAB974C0544994013492B949F8E3812CD3EE4@EXMAIL03.scwf.nsc.com>
Date: Tue, 24 Jan 2012 22:21:39 -0800
From: "Kim, Milo" <Milo.Kim@...com>
To: "Andrew Morton" <akpm@...ux-foundation.org>
cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Richard Purdie" <rpurdie@...ys.net>
Subject: RE: [PATCH] backlight: add new lp855x backlight driver
-----Original Message-----
From: Andrew Morton [mailto:akpm@...ux-foundation.org]
Sent: Saturday, January 21, 2012 9:24 AM
To: Kim, Milo
Cc: linux-kernel@...r.kernel.org; Kim, Milo; Richard Purdie
Subject: Re: [PATCH] backlight: add new lp855x backlight driver
On Thu, 5 Jan 2012 23:00:24 -0800
"Kim, Milo" <Milo.Kim@...com> wrote:
>> + len = snprintf(buf, sizeof(buf), "%s\n", help);
> `len' should have type size_t.
>> + if (len > sizeof(buf))
>> + len = sizeof(buf);
> Here you could use max(). But a better approach is to form the output
> using scnprintf().
This code can be replaced with scnprintf().
But in the updated driver, debugfs nodes will be removed.
The 'chip_id' and 'bl_ctl_mode' will be moved to the lp855x device attributes.
And register access is not necessary in lp855x driver.
>> +static char *lp855x_parse_register_cmd(const char *cmd, u8 *byte)
>> +{
>> + char tmp[10];
>> + char *blank;
>> + unsigned long arg;
>> +
>> + blank = strchr(cmd, ' ');
>> + memset(tmp, 0x0, sizeof(tmp));
>> + memcpy(tmp, cmd, blank - cmd);
>> +
>> + if (strict_strtol(tmp, 16, &arg) < 0)
>> + return NULL;
> Gee, what's all this code doing? Please add a nice comment explaining
> what the input format is, and what this function is trying to do with
> it.
> I worry about what it does when strchr() returns NULL!
Will be removed in the updated driver.
>> +static ssize_t lp855x_ctrl_register(struct file *file,
>> + const char __user *userbuf, size_t count,
>> + loff_t *ppos)
>> +{
>> + char mode, buf[20];
>> + char *pos, *pos2;
>> + u8 i, arg1, arg2, val;
>> + struct lp855x *lp = file->private_data;
>> +
>> + if (copy_from_user(buf, userbuf, min(count, sizeof(buf))))
>> + return -EFAULT;
> Looks risky. If count>sizeof(buf), this will quietly truncate the
> user's input. It would be much better to reject the input in this
> case.
Will be removed in the updated driver.
>> +static ssize_t lp855x_get_chipid(struct file *file, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct lp855x *lp = file->private_data;
>> + char buf[10];
>> + unsigned int len;
>> +
>> + len = snprintf(buf, sizeof(buf), "%s\n", lp->chipid);
>> +
>> + if (len > sizeof(buf))
>> + len = sizeof(buf);
> See above.
This function will be moved to the lp855x device attribute.
And snprintf() and sizeof(buf) will be replaced with scnprintf().
>> + return simple_read_from_buffer(userbuf, count, ppos, buf, len);
>> +}
>> +
>> +static ssize_t lp855x_get_bl_mode(struct file *file, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char buf[20];
>> + unsigned int len;
>> + char *strmode = NULL;
>> + struct lp855x *lp = file->private_data;
>> + enum lp855x_brightness_ctrl_mode mode = lp->pdata->mode;
>> +
>> + if (mode == PWM_BASED)
>> + strmode = "pwm based";
>> + else if (mode == REGISTER_BASED)
>> + strmode = "register based";
>> +
>> + len = snprintf(buf, sizeof(buf), "%s\n", strmode);
>> +
>> + if (len > sizeof(buf))
>> + len = sizeof(buf);
> More...
This function also will be moved to the device attribute.
And snprintf() and sizeof(buf) will be replaced with scnprintf().
>> +static void lp855x_create_debugfs(struct lp855x *lp)
>> +{
>> + struct debug_dentry *dd = &lp->dd;
>> +
>> + dd->dir = debugfs_create_dir("lp855x", NULL);
>> +
>> + dd->reg = debugfs_create_file("registers", S_IWUSR | S_IRUGO,
>> + dd->dir, lp, &dbg_registers_fops);
>> +
>> + dd->chip = debugfs_create_file("chip_id", S_IRUGO,
>> + dd->dir, lp, &dbg_chip_fops);
>> +
>> + dd->blmode = debugfs_create_file("bl_ctl_mode", S_IRUGO,
>> + dd->dir, lp, &dbg_blmode_fops);
> Error checking?
Return code should be checked.
But the 'register' node will be removed in new lp855x driver patch.
The 'chip_id' and 'bl_ctl_mode' will be moved to lp855x device attribute.
>> +static void lp855x_init_device(struct lp855x *lp)
>> +{
>> + u8 val, addr;
>> + int i, ret;
>> + struct lp855x_platform_data *pd = lp->pdata;
>> +
>> + val = pd->initial_brightness;
>> + ret = lp855x_write_byte(lp, BRIGHTNESS_CTRL, val);
>> +
>> + val = pd->device_control;
>> + ret |= lp855x_write_byte(lp, DEVICE_CTRL, val);
>> +
>> + if (pd->load_new_rom_data && pd->size_program) {
>> + for (i = 0; i < pd->size_program; i++) {
>> + addr = pd->rom_data[i].addr;
>> + val = pd->rom_data[i].val;
>> + if (!lp855x_is_valid_rom_area(lp, addr))
>> + continue;
>> +
>> + ret |= lp855x_write_byte(lp, addr, val);
>> + }
>> + }
>> +
>> + if (ret)
>> + dev_err(lp->dev, "i2c write err\n");
>> +}
> This isn't very good. lp855x_write_byte() can return various -Efoo
> values: -EINVAL, -ENOMEM, etc. But this function can end up
> bitwise-ORing those errnos together, thus producing a completely new
> (and wrong) errno.
> That's not a big problem in this case, because that errno is simply
> dropped on the floor. However it would be more useful if the errno
> were reported to the operator in that dev_err() call.
I totally agree with your opinion.
These return code should not be manipulated and returned on xxx_probe().
It will be fixed in new driver patch.
>> +static int lp855x_backlight_register(struct lp855x *lp)
>> +{
>> + struct backlight_device *bl;
>> + struct backlight_properties props;
>> + const char *name = lp->pdata->name;
>> +
>> + if (!name)
>> + return -ENODEV;
>> +
>> + props.brightness = lp->pdata->initial_brightness;
>> + props.max_brightness =
>> + (lp->pdata->max_brightness < lp->pdata->initial_brightness) ?
>> + 255 : lp->pdata->max_brightness;
>> +
>> + bl = backlight_device_register(name, lp->dev, lp,
>> + &lp855x_bl_ops, &props);
>> + if (IS_ERR(bl))
>> + return -EIO;
> If `lb' contains an errno, we should return that errno to the caller
> rather than unconditionally overwriting it with -EIO?
That code will be changed below.
If (IS_ERR(bl))
return PTR_ERR(bl);
I appreciate your help and detailed advice.
Updated lp855x driver will be patched as [PATCH v2].
Thanks & BR
Milo -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists