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

From: Jan Kratochvil <redacted>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jiri Kosina <redacted>, Anssi Hannula <redacted>,
    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 <redacted>

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 <redacted>
Signed-off-by: Jiri Kosina <redacted>

---
 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

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

Hi Jan,

On Tuesday 01 May 2007 19:15, Jan Kratochvil wrote:

From: Jan Kratochvil <redacted>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jiri Kosina <redacted>, Anssi Hannula <redacted>,
    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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

 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;
 	}
 

On Tuesday 01 May 2007 19:20, Jiri Kosina wrote:

On Wed, 2 May 2007, Jan Kratochvil wrote:

quoted

  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

Hi Dmitry,

On 5/2/07, Dmitry Torokhov [off-list ref] wrote:

Hi Jan,

On Tuesday 01 May 2007 19:15, Jan Kratochvil wrote:

quoted

From: Jan Kratochvil <redacted>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jiri Kosina <redacted>, Anssi Hannula <

anssi.hannula@gmail.com>,

quoted

    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

quoted

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

quoted

because effect_id is actually input_event.code which is __u16, but on

the

quoted

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

--