Bug 60650

Summary: sound/soc/codecs/wm0010.c: mem leaks on error path
Product: Drivers Reporter: Mikko Rapeli (mikko.rapeli)
Component: Sound(ALSA)Assignee: Jaroslav Kysela (perex)
Status: CLOSED CODE_FIX    
Severity: normal CC: alan, dp
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 3.11rc2 Subsystem:
Regression: No Bisected commit-id:

Description Mikko Rapeli 2013-07-29 20:38:37 UTC
Coverity reports in CID 751483 and 751484:

 346static int wm0010_firmware_load(const char *name, struct snd_soc_codec *codec)
 347{
 348        struct spi_device *spi = to_spi_device(codec->dev);
 349        struct wm0010_priv *wm0010 = snd_soc_codec_get_drvdata(codec);
 350        struct list_head xfer_list;
 351        struct wm0010_boot_xfer *xfer;
 352        int ret;
 353        struct completion done;
 354        const struct firmware *fw;
 355        const struct dfw_binrec *rec;
 356        const struct dfw_inforec *inforec;
 357        u64 *img;
 358        u8 *out, dsp;
 359        u32 len, offset;
 360
 361        INIT_LIST_HEAD(&xfer_list);
 362
 363        ret = request_firmware(&fw, name, codec->dev);
    	1. Condition "ret != 0", taking false branch
 364        if (ret != 0) {
 365                dev_err(codec->dev, "Failed to request application(%s): %d\n",
 366                        name, ret);
 367                return ret;
 368        }
 369
 370        rec = (const struct dfw_binrec *)fw->data;
 371        inforec = (const struct dfw_inforec *)rec->data;
 372        offset = 0;
 373        dsp = inforec->dsp_target;
 374        wm0010->boot_failed = false;
    	2. Condition "!list_empty(&xfer_list)", taking false branch
 375        BUG_ON(!list_empty(&xfer_list));
 376        init_completion(&done);
 377
 378        /* First record should be INFO */
    	3. Condition "rec->command != DFW_CMD_INFO", taking false branch
 379        if (rec->command != DFW_CMD_INFO) {
 380                dev_err(codec->dev, "First record not INFO\r\n");
 381                ret = -EINVAL;
 382                goto abort;
 383        }
 384
    	4. Condition "inforec->info_version != 1", taking false branch
 385        if (inforec->info_version != INFO_VERSION) {
 386                dev_err(codec->dev,
 387                        "Unsupported version (%02d) of INFO record\r\n",
 388                        inforec->info_version);
 389                ret = -EINVAL;
 390                goto abort;
 391        }
 392
    	5. Condition "!!(descriptor.flags & (1 /* 1 << 0 */))", taking true branch
    	6. Condition "!!(descriptor.flags & (1 /* 1 << 0 */))", taking true branch
 393        dev_dbg(codec->dev, "Version v%02d INFO record found\r\n",
 394                inforec->info_version);
 395
 396        /* Check it's a DSP file */
    	7. Condition "dsp != 10", taking false branch
 397        if (dsp != DEVICE_ID_WM0010) {
 398                dev_err(codec->dev, "Not a WM0010 firmware file.\r\n");
 399                ret = -EINVAL;
 400                goto abort;
 401        }
 402
 403        /* Skip the info record as we don't need to send it */
 404        offset += ((rec->length) + 8);
 405        rec = (void *)&rec->data[rec->length];
 406
    	8. Condition "offset < fw->size", taking true branch
 407        while (offset < fw->size) {
    	9. Condition "!!(descriptor.flags & (1 /* 1 << 0 */))", taking true branch
    	10. Condition "!!(descriptor.flags & (1 /* 1 << 0 */))", taking true branch
 408                dev_dbg(codec->dev,
 409                        "Packet: command %d, data length = 0x%x\r\n",
 410                        rec->command, rec->length);
 411                len = rec->length + 8;
 412
 413                out = kzalloc(len, GFP_KERNEL);
    	11. Condition "!out", taking false branch
 414                if (!out) {
 415                        dev_err(codec->dev,
 416                                "Failed to allocate RX buffer\n");
 417                        ret = -ENOMEM;
 418                        goto abort1;
 419                }
 420
    	12. alloc_fn: Storage is returned from allocation function "kzalloc(size_t, gfp_t)". [show details]
    	13. var_assign: Assigning: "img" = storage returned from "kzalloc(len, 208U)".
 421                img = kzalloc(len, GFP_KERNEL);
    	14. Condition "!img", taking false branch
 422                if (!img) {
 423                        dev_err(codec->dev,
 424                                "Failed to allocate image buffer\n");
 425                        ret = -ENOMEM;
 426                        goto abort1;
 427                }
 428
    	15. noescape: Resource "img" is not freed or pointed-to in function "byte_swap_64(u64 *, u64 *, u32)". [show details]
 429                byte_swap_64((u64 *)&rec->command, img, len);
 430
 431                xfer = kzalloc(sizeof(*xfer), GFP_KERNEL);
    	16. Condition "!xfer", taking true branch
 432                if (!xfer) {
 433                        dev_err(codec->dev, "Failed to allocate xfer\n");
 434                        ret = -ENOMEM;
    	17. Jumping to label "abort1"
 435                        goto abort1;
 436                }
 437
 438                xfer->codec = codec;
 439                list_add_tail(&xfer->list, &xfer_list);
 440
 441                spi_message_init(&xfer->m);
 442                xfer->m.complete = wm0010_boot_xfer_complete;
 443                xfer->m.context = xfer;
 444                xfer->t.tx_buf = img;
 445                xfer->t.rx_buf = out;
 446                xfer->t.len = len;
 447                xfer->t.bits_per_word = 8;
 448
 449                if (!wm0010->pll_running) {
 450                        xfer->t.speed_hz = wm0010->sysclk / 6;
 451                } else {
 452                        xfer->t.speed_hz = wm0010->max_spi_freq;
 453
 454                        if (wm0010->board_max_spi_speed &&
 455                           (wm0010->board_max_spi_speed < wm0010->max_spi_freq))
 456                                        xfer->t.speed_hz = wm0010->board_max_spi_speed;
 457                }
 458
 459                /* Store max usable spi frequency for later use */
 460                wm0010->max_spi_freq = xfer->t.speed_hz;
 461
 462                spi_message_add_tail(&xfer->t, &xfer->m);
 463
 464                offset += ((rec->length) + 8);
 465                rec = (void *)&rec->data[rec->length];
 466
 467                if (offset >= fw->size) {
 468                        dev_dbg(codec->dev, "All transfers scheduled\n");
 469                        xfer->done = &done;
 470                }
 471
 472                ret = spi_async(spi, &xfer->m);
 473                if (ret != 0) {
 474                        dev_err(codec->dev, "Write failed: %d\n", ret);
 475                        goto abort1;
 476                }
 477
 478                if (wm0010->boot_failed) {
 479                        dev_dbg(codec->dev, "Boot fail!\n");
 480                        ret = -EINVAL;
 481                        goto abort1;
 482                }
 483        }
 484
 485        wait_for_completion(&done);
 486
 487        ret = 0;
 488
 489abort1:
    	18. Condition "!list_empty(&xfer_list)", taking false branch
 490        while (!list_empty(&xfer_list)) {
 491                xfer = list_first_entry(&xfer_list, struct wm0010_boot_xfer,
 492                                        list);
 493                kfree(xfer->t.rx_buf);
 494                kfree(xfer->t.tx_buf);
 495                list_del(&xfer->list);
 496                kfree(xfer);
 497        }
 498
 499abort:
 500        release_firmware(fw);
    	CID 751483: Resource leak (RESOURCE_LEAK) [select issue]
    	
CID 751484 (#1 of 1): Resource leak (RESOURCE_LEAK)
19. leaked_storage: Variable "img" going out of scope leaks the storage it points to.
 501        return ret;
 502}

It seems fix would be to assign allocated temporary buffers to xfer->t.rx_buf
and xfer->t.tx_buf earlier since they get kfree()'d on error path:

--- a/sound/soc/codecs/wm0010.c
+++ b/sound/soc/codecs/wm0010.c
@@ -417,6 +417,7 @@ static int wm0010_firmware_load(const char *name, struct snd
_soc_codec *codec)
                        ret = -ENOMEM;
                        goto abort1;
                }
+               xfer->t.rx_buf = out;
 
                img = kzalloc(len, GFP_KERNEL);
                if (!img) {
@@ -425,6 +426,7 @@ static int wm0010_firmware_load(const char *name, struct snd_soc_codec *codec)
                        ret = -ENOMEM;
                        goto abort1;
                }
+               xfer->t.tx_buf = img;
 
                byte_swap_64((u64 *)&rec->command, img, len);
 
@@ -441,8 +443,6 @@ static int wm0010_firmware_load(const char *name, struct snd_soc_codec *codec)
                spi_message_init(&xfer->m);
                xfer->m.complete = wm0010_boot_xfer_complete;
                xfer->m.context = xfer;
-               xfer->t.tx_buf = img;
-               xfer->t.rx_buf = out;
                xfer->t.len = len;
                xfer->t.bits_per_word = 8;
Comment 1 dp 2013-07-31 09:15:59 UTC
Sent a fix upstream.