[PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

[PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

[Jan Kratochvil]

From: Jan Kratochvil <honza@jikos.cz>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>, Anssi Hannula <anssi.hannula@gmail.com>,
    linux-input@atrey.karlin.mff.cuni.cz, linux-kernel@vger.kernel.org
Subject: [PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

Hi,
  i found a bug in ff-memless.c so i fixed it. As you can see I am 
doing check if effect_id is less then 0, but I am aware that it is useless 
because effect_id is actually input_event.code which is __u16, but on the 
other side as long as the effect_id is int I feel it is correct to check 
whether it is > 0. 

Jan.


From: Jan Kratochvil <honza@jikos.cz>

input: playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

To reproduce this bug modify fftest to play effect with code > 15 and 
try to play this effect on device which is implemented using ff-memless.
ml_ff_playback() will try to access ml->states array over the boundary (array
is statically allocated to contain FF_EFFECTS fields). 

Signed-off-by: Jan Kratochvil <honza@jikos.cz>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>

---
 drivers/input/ff-memless.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index d226d93..bc546a6 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -396,7 +396,14 @@ static void ml_ff_set_gain(struct input_
 static int ml_ff_playback(struct input_dev *dev, int effect_id, int value)
 {
 	struct ml_device *ml = dev->ff->private;
-	struct ml_effect_state *state = &ml->states[effect_id];
+	struct ml_effect_state *state;
+	
+	if (effect_id < 0 || effect_id >= FF_MEMLESS_EFFECTS) {
+		printk(KERN_ERR "Effect id %d is out of range!\n", effect_id);
+		return -EINVAL;
+	}
+
+	state = &ml->states[effect_id];
 
 	spin_lock_bh(&ml->timer_lock);
 
-- 
1.4.3.4

Re: [PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

[Jiri Kosina]

On Wed, 2 May 2007, Jan Kratochvil wrote:

>   i found a bug in ff-memless.c so i fixed it. As you can see I am doing 
> check if effect_id is less then 0, but I am aware that it is useless 
> because effect_id is actually input_event.code which is __u16, but on 
> the other side as long as the effect_id is int I feel it is correct to 
> check whether it is > 0.

Dmitry,

actually, looking at the code - is there any particular reason for the 
inconsistency between the types used in struct input_event ( __u16 type; 
__u16 code) and in the rest of the code (basically unsigned int 
everywhere)?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

[Dmitry Torokhov]

Hi Jan,

On Tuesday 01 May 2007 19:15, Jan Kratochvil wrote:
> From: Jan Kratochvil <honza@jikos.cz>
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>, Anssi Hannula <anssi.hannula@gmail.com>,
>     linux-input@atrey.karlin.mff.cuni.cz, linux-kernel@vger.kernel.org
> Subject: [PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow
> 
> Hi,
>   i found a bug in ff-memless.c so i fixed it. As you can see I am 
> doing check if effect_id is less then 0, but I am aware that it is useless 
> because effect_id is actually input_event.code which is __u16, but on the 
> other side as long as the effect_id is int I feel it is correct to check 
> whether it is > 0. 
>

Well spotted, thanks. However I think that the check should be moved up
the stack, into force feedback core. What do you think about the patch
below?

-- 
Dmitry


Input: force feedback - make sure effect is present before playing

Make sure that requested effect id is not out of range for the
device and that effect is present before requesting device to
play it.

Reported-by: Jan Kratochvil <honza@jikos.cz>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/ff-core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

Index: work/drivers/input/ff-core.c
===================================================================
--- work.orig/drivers/input/ff-core.c
+++ work/drivers/input/ff-core.c
@@ -281,7 +281,8 @@ int input_ff_event(struct input_dev *dev
 		break;
 
 	default:
-		ff->playback(dev, code, value);
+		if (check_effect_access(ff, code, NULL) == 0)
+			ff->playback(dev, code, value);
 		break;
 	}
 

Re: [PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

[Dmitry Torokhov]

On Tuesday 01 May 2007 19:20, Jiri Kosina wrote:
> On Wed, 2 May 2007, Jan Kratochvil wrote:
> 
> >   i found a bug in ff-memless.c so i fixed it. As you can see I am doing 
> > check if effect_id is less then 0, but I am aware that it is useless 
> > because effect_id is actually input_event.code which is __u16, but on 
> > the other side as long as the effect_id is int I feel it is correct to 
> > check whether it is > 0.
> 
> Dmitry,
> 
> actually, looking at the code - is there any particular reason for the 
> inconsistency between the types used in struct input_event ( __u16 type; 
> __u16 code) and in the rest of the code (basically unsigned int 
> everywhere)?
>

input_event is part of ABI and I guess Vojtech tried to pack it to minimize
memory copying. However it is just easier to use unisgned int elsewhere in
the kernel - you either pass values in registers on on stack using native
word size.
  
-- 
Dmitry

Re: [PATCH] playing ff effect with code greater then FF_EFFECTS can cause buffer overflow

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Hi Dmitry,

On 5/2/07, Dmitry Torokhov <dtor@insightbb.com> wrote:
>
> Hi Jan,
>
> On Tuesday 01 May 2007 19:15, Jan Kratochvil wrote:
> > From: Jan Kratochvil <honza@jikos.cz>
> > To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Jiri Kosina <jkosina@suse.cz>, Anssi Hannula <
> anssi.hannula@gmail.com>,
> >     linux-input@atrey.karlin.mff.cuni.cz, linux-kernel@vger.kernel.org
> > Subject: [PATCH] playing ff effect with code greater then FF_EFFECTS can
> cause buffer overflow
> >
> > Hi,
> >   i found a bug in ff-memless.c so i fixed it. As you can see I am
> > doing check if effect_id is less then 0, but I am aware that it is
> useless
> > because effect_id is actually input_event.code which is __u16, but on
> the
> > other side as long as the effect_id is int I feel it is correct to check
> > whether it is > 0.
> >
>
> Well spotted, thanks. However I think that the check should be moved up
> the stack, into force feedback core. What do you think about the patch
> below?


Ok by me.

Jan Kratochvil

--
> Dmitry
>
>
> Input: force feedback - make sure effect is present before playing
>
> Make sure that requested effect id is not out of range for the
> device and that effect is present before requesting device to
> play it.
>
> Reported-by: Jan Kratochvil <honza@jikos.cz>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
> drivers/input/ff-core.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletion(-)
>
> Index: work/drivers/input/ff-core.c
> ===================================================================
> --- work.orig/drivers/input/ff-core.c
> +++ work/drivers/input/ff-core.c
> @@ -281,7 +281,8 @@ int input_ff_event(struct input_dev *dev
>                 break;
>
>         default:
> -               ff->playback(dev, code, value);
> +               if (check_effect_access(ff, code, NULL) == 0)
> +                       ff->playback(dev, code, value);
>                 break;
>         }
>
>
>

[-- Attachment #2: Type: text/html, Size: 3421 bytes --]