linux-kernel - re: scsi: ufs: Try to save power mode change and UIC cmd completion timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 9 Nov 2020 13:21:51 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     Can Guo <cang@...eaurora.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Stanley Chu <stanley.chu@...iatek.com>
Cc:     "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: re: scsi: ufs: Try to save power mode change and UIC cmd completion
 timeout

Hi

Static analysis with Coverity on linux-next has detected a potential
null pointer deference issue with commit:

commit 0f52fcb99ea2738a0a0f28e12cf4dd427069dd2a
Author: Can Guo <cang@...eaurora.org>
Date:   Mon Nov 2 22:24:40 2020 -0800

    scsi: ufs: Try to save power mode change and UIC cmd completion timeout


The analysis is as follows:

4925 static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32
intr_status)
4926 {
4927        irqreturn_t retval = IRQ_NONE;
4928

    1. Condition intr_status & 1024, taking true branch.
    2. Condition hba->active_uic_cmd, taking false branch.
    3. var_compare_op: Comparing hba->active_uic_cmd to null implies
that hba->active_uic_cmd might be null.

4929        if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
4930                hba->active_uic_cmd->argument2 |=
4931                        ufshcd_get_uic_cmd_result(hba);
4932                hba->active_uic_cmd->argument3 =
4933                        ufshcd_get_dme_attr_val(hba);
4934                if (!hba->uic_async_done)
4935                        hba->active_uic_cmd->cmd_active = 0;
4936                complete(&hba->active_uic_cmd->done);
4937                retval = IRQ_HANDLED;
4938        }
4939

    4. Condition intr_status & (112U /* (0x40 | 0x20) | 0x10 */), taking
true branch.
    5. Condition hba->uic_async_done, taking true branch.

4940        if ((intr_status & UFSHCD_UIC_PWR_MASK) &&
hba->uic_async_done) {

Dereference after null check (FORWARD_NULL)
    6. var_deref_op: Dereferencing null pointer hba->active_uic_cmd.

4941                hba->active_uic_cmd->cmd_active = 0;
4942                complete(hba->uic_async_done);
4943                retval = IRQ_HANDLED;
4944        }
4945

Line 4929 checks to see if hba->active_uic_cmd is null, so there is a
potential it may be null.  However, on line 4941 hba->active_uic_cmd is
being dereferenced without a null check, so Coverity has flagged this is
a potential null pointer dereference issue.

If it is null, then cmd_active shouldn't be assigned, but I'm unsure if
this is a false positive warning and/or what the ramifications of not
seeting cmd_active to zero is if hba->active_uic_cmd is null.

Colin