xorl %eax, %eax

Linux kernel ff-memless Signedness Issue

leave a comment »

Credits for this bug goes to Jussi Kivilinna. This issue was fixed in 2.6.30-rc6 according to its ChangeLog and affects Linux kernel from 2.6.19 to 2.6.30-rc5 releases. The vulnerable function at drivers/input/ff-memless.c is the following (from 2.6.30-rc5 kernel):

224 /*
225  * Combine two effects and apply gain.
226  */
227 static void ml_combine_effects(struct ff_effect *effect,
228                                struct ml_effect_state *state,
229                                int gain)
230 {
231         struct ff_effect *new = state->effect;
232         unsigned int strong, weak, i;
233         int x, y;
        ...
236         switch (new->type) {
        ...
255         case FF_RUMBLE:
256                 strong = new->u.rumble.strong_magnitude * gain / 0xffff;
257                 weak = new->u.rumble.weak_magnitude * gain / 0xffff;
258                 effect->u.rumble.strong_magnitude =
259                         min(strong + effect->u.rumble.strong_magnitude,
260                             0xffffU);
261                 effect->u.rumble.weak_magnitude =
262                         min(weak + effect->u.rumble.weak_magnitude, 0xffffU);
263                 break;
        ...
283 }

This happens in other cases as well but let’s examine only this one. This is a driver that is used to force feedback support on memoryless devices (ff-memless). This function is used to combine two effects passed as arguments, and apply gain also passed as argument (it says it at the comments). The first argument is a pointer to an ff_effect structure, this is defined in include/linux/input.h like this:

907 /**
908  * struct ff_effect - defines force feedback effect
909  * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
910  *      FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
911  * @id: an unique id assigned to an effect
912  * @direction: direction of the effect
913  * @trigger: trigger conditions (struct ff_trigger)
914  * @replay: scheduling of the effect (struct ff_replay)
915  * @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
916  *      ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
917  *      defining effect parameters
918  *
919  * This structure is sent through ioctl from the application to the driver.
920  * To create a new effect application should set its @id to -1; the kernel
921  * will return assigned @id which can later be used to update or delete
922  * this effect.
923  *
924  * Direction of the effect is encoded as follows:
925  *      0 deg -> 0x0000 (down)
926  *      90 deg -> 0x4000 (left)
927  *      180 deg -> 0x8000 (up)
928  *      270 deg -> 0xC000 (right)
929  */
930 struct ff_effect {
931         __u16 type;
932         __s16 id;
933         __u16 direction;
934         struct ff_trigger trigger;
935         struct ff_replay replay;
936
937         union {
938                 struct ff_constant_effect constant;
939                 struct ff_ramp_effect ramp;
940                 struct ff_periodic_effect periodic;
941                 struct ff_condition_effect condition[2]; /* One for each axis */
942                 struct ff_rumble_effect rumble;
943         } u;
944 };


I didn’t stripped out the comments since they provide excellent documentation that we need to know in order to proceed. Its second argument is defined in drivers/input/ff-memless.c and is this:

50 struct ml_effect_state {
51         struct ff_effect *effect;
52         unsigned long flags;    /* effect state (STARTED, PLAYING, etc) */
53         int count;              /* loop count of the effect */
54         unsigned long play_at;  /* start time */
55         unsigned long stop_at;  /* stop time */
56         unsigned long adj_at;   /* last time the effect was sent */
57 };

When u.rumble.strong_magnitude which is a ff_rumble_effect structure defined in include/linux/input.h like that:

894 /**
895  * struct ff_rumble_effect - defines parameters of a periodic force-feedback effect
896  * @strong_magnitude: magnitude of the heavy motor
897  * @weak_magnitude: magnitude of the light one
898  *
899  * Some rumble pads have two motors of different weight. Strong_magnitude
900  * represents the magnitude of the vibration generated by the heavy one.
901  */
902 struct ff_rumble_effect {
903         __u16 strong_magnitude;
904         __u16 weak_magnitude;
905 };

is set to values larger than 0x8000, then the calculations at lines 256 and 257 (and on some other cases as well) will result in type promotion of strong_magnitude which is unsigned short (line 903) to a signed integer, since gain has the largest data type in the expression and it is of that type (line 229). The subsequent calls to min() will result in truncation of that signed integer to unsigned short variables like strong_magnitude and weak_magnitude (lines 258 and 261). This means that if the result was a large negative integer, they will be containing 0xffff. To fix this bug, the following patch was applied:

                    struct ml_effect_state *state,
-                   int gain)
+                   unsigned int gain)
 {
 struct ff_effect *new = state->effect;

This way gain will not promote the result to a signed integer, it will promote it to an unsigned one, thus no large negative numbers.

Written by xorl

May 20, 2009 at 16:20

Posted in bugs, linux

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s