| 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: <20110513192710.5b4791ca@lxorguk.ukuu.org.uk>
Date: Fri, 13 May 2011 19:27:10 +0100
From: Alan Cox <alan@...rguk.ukuu.org.uk>
To: Chris@...r.kernel.org, Hudson@...r.kernel.org
Cc: dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, Chris Hudson <chudson@...nix.com>
Subject: Re: [PATCH 1/1] input: Add support for Kionix KXTJ9 accelerometer
> + * The following table lists the maximum appropriate poll interval for each
> + * available output data rate.
Surely this can be static const
> + */
> +struct {
> + unsigned int cutoff;
> + u8 mask;
> +} kxtj9_odr_table[] = {
> + {
> +static int kxtj9_i2c_read(struct kxtj9_data *tj9, u8 addr, u8 *data, int len)
> +{
> + int err;
> +
> + struct i2c_msg msgs[] = {
> + {
> + .addr = tj9->client->addr,
> + .flags = tj9->client->flags,
> + .len = 1,
> + .buf = &addr,
> + },
> + {
> + .addr = tj9->client->addr,
> + .flags = tj9->client->flags | I2C_M_RD,
> + .len = len,
> + .buf = data,
> + },
> + };
> + err = i2c_transfer(tj9->client->adapter, msgs, 2);
> +
> + if (err != 2)
> + dev_err(&tj9->client->dev, "read transfer error\n");
Just return ret and check it with < 0 meaning error
> +static int kxtj9_i2c_write(struct kxtj9_data *tj9, u8 addr, u8 *data, int len)
> +{
> + int err;
> + int i;
> + u8 buf[len + 1];
> +
> + struct i2c_msg msgs[] = {
> + {
> + .addr = tj9->client->addr,
> + .flags = tj9->client->flags,
> + .len = len + 1,
> + .buf = buf,
> + },
> + };
> +
> + buf[0] = addr;
> + for (i = 0; i < len; i++)
> + buf[i + 1] = data[i];
memcpy is quite useful and the kernel one inlines nicely
> +int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
> +{
> + int err;
> + u8 shift;
> + u8 buf;
> + u8 out;
> +
> + switch (new_g_range) {
> + case KXTJ9_G_2G:
> + shift = SHIFT_ADJ_2G;
> + break;
> + case KXTJ9_G_4G:
> + shift = SHIFT_ADJ_4G;
> + break;
> + case KXTJ9_G_8G:
> + shift = SHIFT_ADJ_8G;
> + break;
> + default:
> + dev_err(&tj9->client->dev, "invalid g range request\n");
> + return -EINVAL;
So a user can fill the system log ? not good
> + }
> +
> + out = (tj9->resume[RES_CTRL_REG1] & 0xE7) | new_g_range;
> + if (shift != tj9->pdata->shift_adj) {
> + if (atomic_read(&tj9->enabled)) {
> + buf = 0;
> + err = kxtj9_i2c_write(tj9, CTRL_REG1, &buf, 1);
> + if (err < 0)
> + return err;
> + err = kxtj9_i2c_write(tj9, CTRL_REG1, &out, 1);
> + if (err < 0)
> + return err;
> + }
You seem to have no locking on parallel actions and a rather meaningles
atomic
> + }
> +
> + tj9->resume[RES_CTRL_REG1] = out;
> + tj9->pdata->shift_adj = shift;
and unlocked too
In fact the locking seems to be utterly busted everywhere in the driver.
> +static void kxtj9_device_power_off(struct kxtj9_data *tj9)
> +{
> + int err;
> + u8 buf;
> +
> + buf = tj9->resume[RES_CTRL_REG1] & PC1_OFF;
> + err = kxtj9_i2c_write(tj9, CTRL_REG1, &buf, 1);
> + if (err < 0)
> + dev_err(&tj9->client->dev, "soft power off failed\n");
> + disable_irq(tj9->irq);
What if the IRQ is shared ?
> +static irqreturn_t kxtj9_isr(int irq, void *dev)
> +{
> + struct kxtj9_data *tj9 = dev;
> +
> + disable_irq_nosync(irq);
> + schedule_work(&tj9->irq_work);
> +
> + return IRQ_HANDLED;
> +}
Threaded irq will sort this lot out for you
> +static ssize_t kxtj9_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct kxtj9_data *tj9 = i2c_get_clientdata(client);
> + int val = simple_strtoul(buf, NULL, 10);
> + if (val)
> + kxtj9_enable(tj9);
> + else
> + kxtj9_disable(tj9);
You seem to have no locking on this either
> + return count;
> +}
> +static DEVICE_ATTR(delay, S_IRUGO|S_IWUSR, kxtj9_delay_show, kxtj9_delay_store);
> +static DEVICE_ATTR(enable, S_IRUGO|S_IWUSR, kxtj9_enable_show,
> + kxtj9_enable_store);
> +static DEVICE_ATTR(res, S_IRUGO|S_IWUSR, kxtj9_res_show, kxtj9_res_store);
> +static DEVICE_ATTR(drdyenable, S_IRUGO|S_IWUSR, NULL, kxtj9_drdyenable_store);
> +static DEVICE_ATTR(grange, S_IRUGO|S_IWUSR, kxtj9_grange_show,
> + kxtj9_grange_store);
These should probably come from platform data
> +static int __devinit kxtj9_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int err = -1;
> + struct kxtj9_data *tj9 = kzalloc(sizeof(*tj9), GFP_KERNEL);
> + if (tj9 == NULL) {
> + dev_err(&client->dev,
> + "failed to allocate memory for module data\n");
> + err = -ENOMEM;
> + goto err0;
> + }
> + if (client->dev.platform_data == NULL) {
> + dev_err(&client->dev, "platform data is NULL; exiting\n");
> + err = -ENODEV;
> + goto err0;
Leaks memory
> + }
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev, "client not i2c capable\n");
> + err = -ENODEV;
Ditto
> + memcpy(tj9->pdata, client->dev.platform_data, sizeof(*tj9->pdata));
> + if (tj9->pdata->init) {
> + err = tj9->pdata->init();
> + if (err < 0)
> + goto err2;
> + }
> +
> + tj9->irq = gpio_to_irq(tj9->pdata->gpio);
What if it's not on a gpio. Probably better any gpio and irq conversion is
done by the platform callback/init code
And the sysfs nodes need documentation.
Overall it looks like a lot of input drivers that get posted here
- Nobody has considered locking between actions and sysfs (or multiple
sysfs actions in parallel)
- Enable/disable is implemented in blissful ignorance of what happens if
a disable occurs during a sysfs or other action
Most of the rest looks like it just needs a good polish.
--
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