xorl %eax, %eax

Linux kernel NXP TDA18271 Silicon Tuner Frequency Calculation Integer Overflow

with 2 comments

This bug was initially reported by Michael Krufky and here is the susceptible code as seen in drivers/media/common/tuners/tda18271-fe.c from 2.6.31 release of the Linux kernel.

static int tda18271_set_analog_params(struct dvb_frontend *fe,
                                      struct analog_parameters *params)
{
        struct tda18271_priv *priv = fe->tuner_priv;
        struct tda18271_std_map *std_map = &priv->std;
        struct tda18271_std_map_item *map;
        char *mode;
        int ret;
        u32 freq = params->frequency * 62500;

        priv->mode = TDA18271_ANALOG;

        if (params->mode == V4L2_TUNER_RADIO) {
    ...
        ret = tda18271_tune(fe, map, freq, 0);

        if (tda_fail(ret))
                goto fail;

        priv->frequency = freq;
        priv->bandwidth = 0;
fail:
        return ret;
}

As you can see, ‘freq’ is an unsigned, 32-bit long integer and it is initialized using the ‘params->frequency’ multiplied by 62500. The ‘analog_parameters’ is a structure defined in drivers/media/dvb/dvb-core/dvb_frontend.h header file like that:

struct analog_parameters {
        unsigned int frequency;
        unsigned int mode;
        unsigned int audmode;
        u64 std;
};

Obviously, the multiplication with 62500 could easily result in an integer that would not fit into a 32-bit long unsigned value. To avoid this integer overflow the following patch was applied:

 	char *mode;
 	int ret;
-	u32 freq = params->frequency * 62500;
+	u32 freq = params->frequency * 125 *
+		((params->mode == V4L2_TUNER_RADIO) ? 1 : 1000) / 2;
 
 	priv->mode = TDA18271_ANALOG;
 
 	if (params->mode == V4L2_TUNER_RADIO) {
-		freq = freq / 1000;
 		map = &std_map->fm_radio;
 		mode = "fm";

I wrote about this bug just because I found it a nice example for integer overflows, I’m not sure if it has any security implications, although, it might have.

Written by xorl

December 7, 2009 at 00:33

Posted in bugs, linux

2 Responses

Subscribe to comments with RSS.

  1. I don’t see any change this patch make to avoid integer overflow there. Or is there a difference if one can overflow it multiplying by 62500 or 62 ?

    soft_dist

    December 7, 2009 at 17:48

  2. The problem was that the overflowed value was later being divided by 1000 as you can read in the initial code. This resulted in an invalid value passed to tda18271_tune().
    The patch performs this division immediately and because of this the frequency’s value is always small enough to fit in a 32-bit long integer.

    xorl

    December 7, 2009 at 21:05


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