xc2028: Fix use-after-free bug properly

The commit 8dfbcc4351a0 ("[media] xc2028: avoid use after free") tried
to address the reported use-after-free by clearing the reference.

However, it's clearing the wrong pointer; it sets NULL to
priv->ctrl.fname, but it's anyway overwritten by the next line
memcpy(&priv->ctrl, p, sizeof(priv->ctrl)).

OTOH, the actual code accessing the freed string is the strcmp() call
with priv->fname:
	if (!firmware_name[0] && p->fname &&
	    priv->fname && strcmp(p->fname, priv->fname))
		free_firmware(priv);

where priv->fname points to the previous file name, and this was
already freed by kfree().

For fixing the bug properly, this patch does the following:

- Keep the copy of firmware file name in only priv->fname,
  priv->ctrl.fname isn't changed;
- The allocation is done only when the firmware gets loaded;
- The kfree() is called in free_firmware() commonly

Fixes: commit 8dfbcc4351a0 ('[media] xc2028: avoid use after free')
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/tuners/tuner-xc2028.c b/drivers/media/tuners/tuner-xc2028.c
index 317ef63ee78999d673f85b62af14bed7e664549e..8d96a22647b396c5a8790775a883ff58eabce7ce 100644 (file)
--- a/drivers/media/tuners/tuner-xc2028.c
+++ b/drivers/media/tuners/tuner-xc2028.c
@@ -281,6 +281,14 @@ static void free_firmware(struct xc2028_data *priv)
        int i;
        tuner_dbg("%s called\n", __func__);
 
+       /* free allocated f/w string */
+       if (priv->fname != firmware_name)
+               kfree(priv->fname);
+       priv->fname = NULL;
+
+       priv->state = XC2028_NO_FIRMWARE;
+       memset(&priv->cur_fw, 0, sizeof(priv->cur_fw));
+
        if (!priv->firm)
                return;
 
@@ -291,9 +299,6 @@ static void free_firmware(struct xc2028_data *priv)
 
        priv->firm = NULL;
        priv->firm_size = 0;
-       priv->state = XC2028_NO_FIRMWARE;
-
-       memset(&priv->cur_fw, 0, sizeof(priv->cur_fw));
 }
 
 static int load_all_firmwares(struct dvb_frontend *fe,
@@ -884,9 +889,8 @@ read_not_reliable:
        return 0;
 
 fail:
-       priv->state = XC2028_NO_FIRMWARE;
+       free_firmware(priv);
 
-       memset(&priv->cur_fw, 0, sizeof(priv->cur_fw));
        if (retry_count < 8) {
                msleep(50);
                retry_count++;
@@ -1332,11 +1336,8 @@ static int xc2028_dvb_release(struct dvb_frontend *fe)
        mutex_lock(&xc2028_list_mutex);
 
        /* only perform final cleanup if this is the last instance */
-       if (hybrid_tuner_report_instance_count(priv) == 1) {
+       if (hybrid_tuner_report_instance_count(priv) == 1)
                free_firmware(priv);
-               kfree(priv->ctrl.fname);
-               priv->ctrl.fname = NULL;
-       }
 
        if (priv)
                hybrid_tuner_release_state(priv);
@@ -1399,19 +1400,8 @@ static int xc2028_set_config(struct dvb_frontend *fe, void *priv_cfg)
 
        /*
         * Copy the config data.
-        * For the firmware name, keep a local copy of the string,
-        * in order to avoid troubles during device release.
         */
-       kfree(priv->ctrl.fname);
-       priv->ctrl.fname = NULL;
        memcpy(&priv->ctrl, p, sizeof(priv->ctrl));
-       if (p->fname) {
-               priv->ctrl.fname = kstrdup(p->fname, GFP_KERNEL);
-               if (priv->ctrl.fname == NULL) {
-                       rc = -ENOMEM;
-                       goto unlock;
-               }
-       }
 
        /*
         * If firmware name changed, frees firmware. As free_firmware will
@@ -1426,10 +1416,15 @@ static int xc2028_set_config(struct dvb_frontend *fe, void *priv_cfg)
 
        if (priv->state == XC2028_NO_FIRMWARE) {
                if (!firmware_name[0])
-                       priv->fname = priv->ctrl.fname;
+                       priv->fname = kstrdup(p->fname, GFP_KERNEL);
                else
                        priv->fname = firmware_name;
 
+               if (!priv->fname) {
+                       rc = -ENOMEM;
+                       goto unlock;
+               }
+
                rc = request_firmware_nowait(THIS_MODULE, 1,
                                             priv->fname,
                                             priv->i2c_props.adap->dev.parent,