CVE-2011-2700: Linux kernel si4713-i2c Buffer Overflow

The following issue was reported by Mauro Carvalho Chehab of Red Hat. The buggy code is part of Silicon Labs Si4713 FM Radio Transmitter I2C driver which according to Eugene Teo is only used by N900.

The susceptible code is available in drivers/media/radio/si4713-i2c.c file and here is the exact code snippet…

/*
 * Video4Linux Subdev Interface
 */
/* si4713_s_ext_ctrls - set extended controls value */
static int si4713_s_ext_ctrls(struct v4l2_subdev *sd,
                                struct v4l2_ext_controls *ctrls)
{
   ...
                switch ((ctrls->controls + i)->id) {
                case V4L2_CID_RDS_TX_PS_NAME:
                case V4L2_CID_RDS_TX_RADIO_TEXT:
                        err = si4713_write_econtrol_string(sdev,
                                                        ctrls->controls + i);
                        break;
   ...
        return 0;
}
   ...
static const struct v4l2_subdev_core_ops si4713_subdev_core_ops = {
        .queryctrl      = si4713_queryctrl,
        .g_ext_ctrls    = si4713_g_ext_ctrls,
        .s_ext_ctrls    = si4713_s_ext_ctrls,
        .g_ctrl         = si4713_g_ctrl,
        .s_ctrl         = si4713_s_ctrl,
        .ioctl          = si4713_ioctl,
}

So, using the si4713_s_ext_ctrls() operation and the ‘V4L2_CID_RDS_TX_PS_NAM’ or ‘V4L2_CID_RDS_TX_RADIO_TEX’ control IDs, a user could reach si4713_write_econtrol_string() function which is shown below.

/* write string property */
static int si4713_write_econtrol_string(struct si4713_device *sdev,
                                struct v4l2_ext_control *control)
{
        struct v4l2_queryctrl vqc;
        int len;
        s32 rval = 0;

        vqc.id = control->id;
        rval = si4713_queryctrl(&sdev->sd, &vqc);
        if (rval < 0)
                goto exit;

        switch (control->id) {
        case V4L2_CID_RDS_TX_PS_NAME: {
                char ps_name[MAX_RDS_PS_NAME + 1];

                len = control->size - 1;
                if (len > MAX_RDS_PS_NAME) {
                        rval = -ERANGE;
                        goto exit;
                }
                rval = copy_from_user(ps_name, control->string, len);
   ...
        case V4L2_CID_RDS_TX_RADIO_TEXT: {
                char radio_text[MAX_RDS_RADIO_TEXT + 1];

                len = control->size - 1;
                if (len > MAX_RDS_RADIO_TEXT) {
                        rval = -ERANGE;
                        goto exit;
                }
                rval = copy_from_user(radio_text, control->string, len);
   ...
exit:
        return rval;
}

As you can see, in both cases the ‘len’ singed integer is initialized by the passed ‘control->size’ and it is only checked against the maximum allowed value. Altough copy_from_user() on x86/x86_64 would prevent any negative values from being used, other architectures might be vulnerable.
Of course, the fix was to add the missing check…

                len = control->size - 1;
-               if (len > MAX_RDS_PS_NAME) {
+               if (len < 0 || len > MAX_RDS_PS_NAME) {
                        rval = -ERANGE;

And equivalently in the second case…

                len = control->size - 1;
-               if (len > MAX_RDS_RADIO_TEXT) {
+               if (len < 0 || len > MAX_RDS_RADIO_TEXT) {
                        rval = -ERANGE;