| 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
| ||
|
Date: Mon, 8 Mar 2010 23:41:55 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Stefan Achatz <stefan_achatz@....de>
Cc: Jiri Kosina <jkosina@...e.cz>,
Jussi Kivilinna <jussi.kivilinna@...et.fi>, wylda@...ny.cz,
Pavel Machek <pavel@...e.cz>,
Alessandro Guido <ag@...ssandroguido.name>,
Tomas Hanak <tomas.hanak@...il.com>,
Jason Noble <nobleja@...ezero.com>, simon.windows@...il.com,
Sean Hildebrand <silverwraithii@...il.com>,
Sid Boyce <sboyce@...eyonder.co.uk>,
Henning Glawe <glaweh@...ian.org>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] Added locks for sysfs attributes and internal data.
Hi Stefan,
On Mon, Mar 08, 2010 at 05:04:29PM +0100, Stefan Achatz wrote:
> From 47678162da8e374d6f132db21d89b718ee5cfbd1 Mon Sep 17 00:00:00 2001
> From: Stefan Achatz <erazor_de@...rs.sourceforge.net>
> Date: Mon, 8 Mar 2010 16:54:19 +0100
> Subject: [PATCH 4/4] Added locks for sysfs attributes and internal data.
>
> Added mutex lock to prevent different processes from accessing
> sysfs attributes at the same time.
> Added spin lock for internal data.
> ---
> drivers/hid/hid-roccat-kone.c | 246 ++++++++++++++++++++++++++++------------
> drivers/hid/hid-roccat-kone.h | 9 +-
> 2 files changed, 179 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 941f5b3..eded7d4 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c
> @@ -223,11 +223,6 @@ static int kone_get_profile(struct usb_device *usb_dev,
> if (number < 1 || number > 5)
> return -EINVAL;
>
> - /*
> - * The manufacturer uses a control message of type class with interface
> - * as recipient and provides the profile number as index parameter.
> - * This is prevented in userspace by function check_ctrlrecip.
> - */
> len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
> | USB_RECIP_INTERFACE | USB_DIR_IN,
> @@ -398,16 +393,18 @@ static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
> static ssize_t kone_sysfs_set_settings(struct device *dev,
> struct device_attribute *attr, char const *buf, size_t size)
> {
> - struct hid_device *hdev = dev_get_drvdata(dev);
> - struct kone_device *kone = hid_get_drvdata(hdev);
> - struct usb_interface *intf = to_usb_interface(dev);
> - struct usb_device *usb_dev = interface_to_usbdev(intf);
> + struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> + struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> int err;
> + unsigned long flags;
>
> if (size != sizeof(struct kone_settings))
> return -EINVAL;
>
> + mutex_lock(&kone->kone_lock);
> err = kone_set_settings(usb_dev, (struct kone_settings const *)buf);
Hmm, this kind of casting tells me that this is binary, not text data
and thus binary sysfs attribute shoudl be used.
> + mutex_unlock(&kone->kone_lock);
> +
> if (err)
> return err;
>
> @@ -415,9 +412,10 @@ static ssize_t kone_sysfs_set_settings(struct device *dev,
> * If we get here, treat buf as okay (apart from checksum) and use value
> * of startup_profile as its at hand
> */
> + spin_lock_irqsave(&kone->actual_lock, flags);
> kone->act_profile = ((struct kone_settings *)buf)->startup_profile;
> - kone->act_profile_valid = 1;
> - kone->act_dpi_valid = 0;
> + kone->act_dpi = -1;
> + spin_unlock_irqrestore(&kone->actual_lock, flags);
>
You don't really need a spinlock to set one integer.
> return sizeof(struct kone_settings);
> }
> @@ -425,10 +423,14 @@ static ssize_t kone_sysfs_set_settings(struct device *dev,
> static ssize_t kone_sysfs_show_settings(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct usb_interface *intf = to_usb_interface(dev);
> - struct usb_device *usb_dev = interface_to_usbdev(intf);
> + struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> + struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> int err;
> +
> + mutex_lock(&kone->kone_lock);
> err = kone_get_settings(usb_dev, (struct kone_settings *)buf);
> + mutex_unlock(&kone->kone_lock);
> +
> if (err)
> return err;
>
> @@ -437,10 +439,14 @@ static ssize_t kone_sysfs_show_settings(struct device *dev,
>
> static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
> {
> - struct usb_interface *intf = to_usb_interface(dev);
> - struct usb_device *usb_dev = interface_to_usbdev(intf);
> + struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> + struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> int err;
> +
> + mutex_lock(&kone->kone_lock);
> err = kone_get_profile(usb_dev, (struct kone_profile *)buf, number);
> + mutex_unlock(&kone->kone_lock);
> +
> if (err)
> return err;
> return sizeof(struct kone_profile);
> @@ -449,15 +455,17 @@ static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
> static ssize_t kone_sysfs_set_profile(struct device *dev, char const *buf,
> size_t size, int number)
> {
> - struct usb_interface *intf = to_usb_interface(dev);
> - struct usb_device *usb_dev = interface_to_usbdev(intf);
> -
> + struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> + struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> int err;
>
> if (size != sizeof(struct kone_profile))
> return -EINVAL;
>
> + mutex_lock(&kone->kone_lock);
> err = kone_set_profile(usb_dev, buf, number);
> + mutex_unlock(&kone->kone_lock);
> +
> if (err)
> return err;
>
> @@ -525,32 +533,68 @@ static ssize_t kone_sysfs_set_profile_5(struct device *dev,
> }
>
> /*
> - * Helper to fill kone_device structure with actual values
> - * On success returns 0
> - * On failure returns errno
> + * Fills act_profile in kone_device.
> + * Also writes result in @result.
> + * Once act_profile is valid, its not getting invalid any more.
> + * Returns on success 0, on failure errno
> */
> -static int kone_device_set_actual_values(struct kone_device *kone,
> - struct device *dev, int both)
> +static int kone_device_set_actual_profile(struct kone_device *kone,
> + struct device *dev, int *result)
> {
> - struct usb_interface *intf = to_usb_interface(dev);
> - struct usb_device *usb_dev = interface_to_usbdev(intf);
> - int err;
> + struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> + int err, temp;
> + unsigned long flags;
>
> - /* first make sure profile is valid */
> - if (!kone->act_profile_valid) {
> - err = kone_get_startup_profile(usb_dev, &kone->act_profile);
> + spin_lock_irqsave(&kone->actual_lock, flags);
> + if (kone->act_profile == -1) {
> + spin_unlock_irqrestore(&kone->actual_lock, flags);
> + err = kone_get_startup_profile(usb_dev, &temp);
> if (err)
> return err;
> - kone->act_profile_valid = 1;
> + spin_lock_irq(&kone->actual_lock);
> + kone->act_profile = temp;
> + spin_unlock_irqrestore(&kone->actual_lock, flags);
You shoudl not be mixing _irq/_irqrestore. Also it is probably not
needed at all. If it is needed then the common style to acquire and
release locks only once in a function - this makes checking whether
lock/unlock is balanced easier.
> + if (result)
> + *result = temp;
> + } else {
> + if (result)
> + *result = kone->act_profile;
> + spin_unlock_irqrestore(&kone->actual_lock, flags);
> }
> + return 0;
> +}
> +
> +/*
> + * Fills act_dpi in kone_device.
> + * Also writes result in @result.
> + * On success returns 0
> + * On failure returns errno
> + */
> +static int kone_device_set_actual_dpi(struct kone_device *kone,
> + struct device *dev, int *result)
> +{
> + struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> + int err, temp;
> + unsigned long flags;
> +
> + kone_device_set_actual_profile(kone, dev, NULL);
>
> - /* then get startup dpi value */
> - if (!kone->act_dpi_valid && both) {
> + spin_lock_irqsave(&kone->actual_lock, flags);
> + if (kone->act_dpi == -1) {
> + spin_unlock_irqrestore(&kone->actual_lock, flags);
So what are we protecting exactly? What if act_dpi will become -1 here
(after you released the spinlock?
> err = kone_get_profile_startup_dpi(usb_dev, kone->act_profile,
> - &kone->act_dpi);
> + &temp);
Stopped looking - locking obviously is bogus and needs to be redone. You
need to decide what exactly needs protection and what critical sections
are.
--
Dmitry
--
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