The uinput device driver (drivers/input/misc/uinput.c) incorrectly
computes the size of new device names. This patch fixes this. The diff
is against 2.6.37 but the bug also exists in the most recent git
sources.
strnlen() never returns negative numbers so the result should be
incremented after checking for zero. Otherwise the test will always be
false. This bug allows empty input device names.
Please CC me, I'm not subscribed to the mailing list.
David
On Sun, Feb 6, 2011 at 5:55 PM, Aristeu Rozanski <[email protected]> wrote:
> and where's the patch? :^)
>
> --
> Aristeu
>
ah, embarrasing.., sorry.
I attached the patchfile.
David
On Sun, Feb 06, 2011 at 05:49:23PM +0100, David Herrmann wrote:
> The uinput device driver (drivers/input/misc/uinput.c) incorrectly
> computes the size of new device names. This patch fixes this. The diff
> is against 2.6.37 but the bug also exists in the most recent git
> sources.
>
> strnlen() never returns negative numbers so the result should be
> incremented after checking for zero. Otherwise the test will always be
> false. This bug allows empty input device names.
>
> Please CC me, I'm not subscribed to the mailing list.
and where's the patch? :^)
--
Aristeu
On Sun, Feb 06, 2011 at 05:57:19PM +0100, David Herrmann wrote:
> On Sun, Feb 6, 2011 at 5:55 PM, Aristeu Rozanski <[email protected]> wrote:
> > and where's the patch? :^)
> >
> > --
> > Aristeu
> >
>
> ah, embarrasing.., sorry.
> I attached the patchfile.
>
> David
> --- old/drivers/input/misc/uinput.c 2011-02-06 17:40:24.951454656 +0100
> +++ new/drivers/input/misc/uinput.c 2011-02-06 17:41:16.747454654 +0100
> @@ -372,8 +372,8 @@
>
> udev->ff_effects_max = user_dev->ff_effects_max;
>
> - size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
> - if (!size) {
> + size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE);
> + if (!size++) {
> retval = -EINVAL;
> goto exit;
> }
Acked-by: Aristeu Rozanski <[email protected]>
--
Aristeu
On Sun, Feb 06, 2011 at 12:23:17PM -0500, Aristeu Rozanski wrote:
> On Sun, Feb 06, 2011 at 05:57:19PM +0100, David Herrmann wrote:
> > On Sun, Feb 6, 2011 at 5:55 PM, Aristeu Rozanski <[email protected]> wrote:
> > > and where's the patch? :^)
> > >
> > > --
> > > Aristeu
> > >
> >
> > ah, embarrasing.., sorry.
> > I attached the patchfile.
> >
> > David
>
> > --- old/drivers/input/misc/uinput.c 2011-02-06 17:40:24.951454656 +0100
> > +++ new/drivers/input/misc/uinput.c 2011-02-06 17:41:16.747454654 +0100
> > @@ -372,8 +372,8 @@
> >
> > udev->ff_effects_max = user_dev->ff_effects_max;
> >
> > - size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
> > - if (!size) {
> > + size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE);
> > + if (!size++) {
> > retval = -EINVAL;
> > goto exit;
> > }
> Acked-by: Aristeu Rozanski <[email protected]>
>
Hmm, not particularly fond with the construct, how about below instead?
Btw, having "Signed-off-by: " from David would be nice.
Thanks.
--
Dmitry
Input: uinput - fix setting up device name
From: David Herrmann <[email protected]>
The check for non-empty device name was botched since we tried to account
for extra space for the terminating zero at the same time. Convert to
kstrndup() to avoid this problem.
Acked-by: Aristeu Rozanski <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/input/misc/uinput.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 82542a1..c0888e3 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -347,8 +347,7 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
{
struct uinput_user_dev *user_dev;
struct input_dev *dev;
- char *name;
- int i, size;
+ int i;
int retval;
if (count != sizeof(struct uinput_user_dev))
@@ -373,19 +372,19 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
udev->ff_effects_max = user_dev->ff_effects_max;
- size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
- if (!size) {
+ /* Ensure name is filled in */
+ if (!user_dev->name[0]) {
retval = -EINVAL;
goto exit;
}
kfree(dev->name);
- dev->name = name = kmalloc(size, GFP_KERNEL);
- if (!name) {
+ dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE,
+ GFP_KERNEL);
+ if (!dev->name) {
retval = -ENOMEM;
goto exit;
}
- strlcpy(name, user_dev->name, size);
dev->id.bustype = user_dev->id.bustype;
dev->id.vendor = user_dev->id.vendor;
> Hmm, not particularly fond with the construct, how about below instead?
>
> Btw, having "Signed-off-by: " from David would be nice.
>
> Thanks.
>
> --
> Dmitry
>
> Input: uinput - fix setting up device name
>
> From: David Herrmann <[email protected]>
>
> The check for non-empty device name was botched since we tried to account
> for extra space for the terminating zero at the same time. Convert to
> kstrndup() to avoid this problem.
>
> Acked-by: Aristeu Rozanski <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> ?drivers/input/misc/uinput.c | ? 13 ++++++-------
> ?1 files changed, 6 insertions(+), 7 deletions(-)
>
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 82542a1..c0888e3 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -347,8 +347,7 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
> ?{
> ? ? ? ?struct uinput_user_dev ?*user_dev;
> ? ? ? ?struct input_dev ? ? ? ?*dev;
> - ? ? ? char ? ? ? ? ? ? ? ? ? ?*name;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? i, size;
> + ? ? ? int ? ? ? ? ? ? ? ? ? ? i;
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? retval;
>
> ? ? ? ?if (count != sizeof(struct uinput_user_dev))
> @@ -373,19 +372,19 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
>
> ? ? ? ?udev->ff_effects_max = user_dev->ff_effects_max;
>
> - ? ? ? size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
> - ? ? ? if (!size) {
> + ? ? ? /* Ensure name is filled in */
> + ? ? ? if (!user_dev->name[0]) {
> ? ? ? ? ? ? ? ?retval = -EINVAL;
> ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ?}
>
> ? ? ? ?kfree(dev->name);
> - ? ? ? dev->name = name = kmalloc(size, GFP_KERNEL);
> - ? ? ? if (!name) {
> + ? ? ? dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?GFP_KERNEL);
> + ? ? ? if (!dev->name) {
> ? ? ? ? ? ? ? ?retval = -ENOMEM;
> ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ?}
> - ? ? ? strlcpy(name, user_dev->name, size);
>
> ? ? ? ?dev->id.bustype = user_dev->id.bustype;
> ? ? ? ?dev->id.vendor ?= user_dev->id.vendor;
>
This is definitely a better solution, yes. Thank you.
Signed-off-by: David Herrmann <[email protected]>
David
On Sun, Feb 06, 2011 at 11:40:17PM -0800, Dmitry Torokhov wrote:
> Hmm, not particularly fond with the construct, how about below instead?
>
> Btw, having "Signed-off-by: " from David would be nice.
>
> Thanks.
>
> --
> Dmitry
>
> Input: uinput - fix setting up device name
>
> From: David Herrmann <[email protected]>
>
> The check for non-empty device name was botched since we tried to account
> for extra space for the terminating zero at the same time. Convert to
> kstrndup() to avoid this problem.
>
> Acked-by: Aristeu Rozanski <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> drivers/input/misc/uinput.c | 13 ++++++-------
> 1 files changed, 6 insertions(+), 7 deletions(-)
>
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 82542a1..c0888e3 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -347,8 +347,7 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
> {
> struct uinput_user_dev *user_dev;
> struct input_dev *dev;
> - char *name;
> - int i, size;
> + int i;
> int retval;
>
> if (count != sizeof(struct uinput_user_dev))
> @@ -373,19 +372,19 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
>
> udev->ff_effects_max = user_dev->ff_effects_max;
>
> - size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
> - if (!size) {
> + /* Ensure name is filled in */
> + if (!user_dev->name[0]) {
> retval = -EINVAL;
> goto exit;
> }
>
> kfree(dev->name);
> - dev->name = name = kmalloc(size, GFP_KERNEL);
> - if (!name) {
> + dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE,
> + GFP_KERNEL);
> + if (!dev->name) {
> retval = -ENOMEM;
> goto exit;
> }
> - strlcpy(name, user_dev->name, size);
>
> dev->id.bustype = user_dev->id.bustype;
> dev->id.vendor = user_dev->id.vendor;
even better
ACK
--
Aristeu
On Sun, Feb 06, 2011 at 11:40:17PM -0800, Dmitry Torokhov wrote:
> On Sun, Feb 06, 2011 at 12:23:17PM -0500, Aristeu Rozanski wrote:
> > On Sun, Feb 06, 2011 at 05:57:19PM +0100, David Herrmann wrote:
> > > On Sun, Feb 6, 2011 at 5:55 PM, Aristeu Rozanski <[email protected]> wrote:
> > > > and where's the patch? :^)
> > > >
> > > > --
> > > > Aristeu
> > > >
> > >
> > > ah, embarrasing.., sorry.
> > > I attached the patchfile.
> > >
> > > David
> >
> > > --- old/drivers/input/misc/uinput.c 2011-02-06 17:40:24.951454656 +0100
> > > +++ new/drivers/input/misc/uinput.c 2011-02-06 17:41:16.747454654 +0100
> > > @@ -372,8 +372,8 @@
> > >
> > > udev->ff_effects_max = user_dev->ff_effects_max;
> > >
> > > - size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
> > > - if (!size) {
> > > + size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE);
> > > + if (!size++) {
> > > retval = -EINVAL;
> > > goto exit;
> > > }
> > Acked-by: Aristeu Rozanski <[email protected]>
> >
>
> Hmm, not particularly fond with the construct, how about below instead?
>
And while we are at it...
--
Dmitry
Input: uinput - use memdup_user() and friends
From: Dmitry Torokhov <[email protected]>
Instead of open-coding copying of data structures from userspace use
memdup_user() and strndup_user(). Note that this introduces change in
behavior because driver used to truncate 'phys' longer than 1024 bytes,
but now it will refuse to set 'phys' that long. Arguably trying to set
such 'phys' is suspect anyways.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/input/misc/uinput.c | 35 ++++++++++-------------------------
1 files changed, 10 insertions(+), 25 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index c0888e3..7f8331f 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -361,14 +361,9 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
dev = udev->dev;
- user_dev = kmalloc(sizeof(struct uinput_user_dev), GFP_KERNEL);
- if (!user_dev)
- return -ENOMEM;
-
- if (copy_from_user(user_dev, buffer, sizeof(struct uinput_user_dev))) {
- retval = -EFAULT;
- goto exit;
- }
+ user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
+ if (!IS_ERR(user_dev))
+ return PTR_ERR(user_dev);
udev->ff_effects_max = user_dev->ff_effects_max;
@@ -621,7 +616,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
struct uinput_ff_upload ff_up;
struct uinput_ff_erase ff_erase;
struct uinput_request *req;
- int length;
char *phys;
retval = mutex_lock_interruptible(&udev->mutex);
@@ -688,24 +682,15 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
retval = -EINVAL;
goto out;
}
- length = strnlen_user(p, 1024);
- if (length <= 0) {
- retval = -EFAULT;
- break;
+
+ phys = strndup_user(p, 1024);
+ if (IS_ERR(phys)) {
+ retval = PTR_ERR(phys);
+ goto out;
}
+
kfree(udev->dev->phys);
- udev->dev->phys = phys = kmalloc(length, GFP_KERNEL);
- if (!phys) {
- retval = -ENOMEM;
- break;
- }
- if (copy_from_user(phys, p, length)) {
- udev->dev->phys = NULL;
- kfree(phys);
- retval = -EFAULT;
- break;
- }
- phys[length - 1] = '\0';
+ udev->dev->phys = phys;
break;
case UI_BEGIN_FF_UPLOAD:
On Mon, Feb 07, 2011 at 11:38:48PM -0800, Dmitry Torokhov wrote:
> Input: uinput - use memdup_user() and friends
>
> From: Dmitry Torokhov <[email protected]>
>
> Instead of open-coding copying of data structures from userspace use
> memdup_user() and strndup_user(). Note that this introduces change in
> behavior because driver used to truncate 'phys' longer than 1024 bytes,
> but now it will refuse to set 'phys' that long. Arguably trying to set
> such 'phys' is suspect anyways.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> drivers/input/misc/uinput.c | 35 ++++++++++-------------------------
> 1 files changed, 10 insertions(+), 25 deletions(-)
>
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index c0888e3..7f8331f 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -361,14 +361,9 @@ static int uinput_setup_device(struct uinput_device *udev, const char __user *bu
>
> dev = udev->dev;
>
> - user_dev = kmalloc(sizeof(struct uinput_user_dev), GFP_KERNEL);
> - if (!user_dev)
> - return -ENOMEM;
> -
> - if (copy_from_user(user_dev, buffer, sizeof(struct uinput_user_dev))) {
> - retval = -EFAULT;
> - goto exit;
> - }
> + user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
> + if (!IS_ERR(user_dev))
> + return PTR_ERR(user_dev);
>
> udev->ff_effects_max = user_dev->ff_effects_max;
>
> @@ -621,7 +616,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> struct uinput_ff_upload ff_up;
> struct uinput_ff_erase ff_erase;
> struct uinput_request *req;
> - int length;
> char *phys;
>
> retval = mutex_lock_interruptible(&udev->mutex);
> @@ -688,24 +682,15 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> retval = -EINVAL;
> goto out;
> }
> - length = strnlen_user(p, 1024);
> - if (length <= 0) {
> - retval = -EFAULT;
> - break;
> +
> + phys = strndup_user(p, 1024);
> + if (IS_ERR(phys)) {
> + retval = PTR_ERR(phys);
> + goto out;
> }
> +
> kfree(udev->dev->phys);
> - udev->dev->phys = phys = kmalloc(length, GFP_KERNEL);
> - if (!phys) {
> - retval = -ENOMEM;
> - break;
> - }
> - if (copy_from_user(phys, p, length)) {
> - udev->dev->phys = NULL;
> - kfree(phys);
> - retval = -EFAULT;
> - break;
> - }
> - phys[length - 1] = '\0';
> + udev->dev->phys = phys;
> break;
>
> case UI_BEGIN_FF_UPLOAD:
looks good to me
Acked-by: Aristeu Rozanski <[email protected]>
--
Aristeu