linux-kernel - re: scsi: sg: Replace sg_allow

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]

Message-ID: <1f63da00-ef1e-25c9-1494-61ad43135f9e@canonical.com>
Date:   Thu, 11 Mar 2021 11:07:55 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     Doug Gilbert <dgilbert@...erlog.com>
Cc:     "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: scsi: sg: Replace sg_allow_access()

Hi,

Static analysis on linux-next with Coverity has detected an issue in
drivers/scsi/sg.c in function sg_remove_sfp_usercontext with the
following recent commit:

commit 0c32296d73ec5dec64729eb555f1a29ded8a7272
Author: Douglas Gilbert <dgilbert@...erlog.com>
Date:   Fri Feb 19 21:00:28 2021 -0500

    scsi: sg: Replace sg_allow_access()

The analysis is as follows:

3913        if (unlikely(sfp != e_sfp))
3914                SG_LOG(1, sfp, "%s: xa_erase() return unexpected\n",
3915                       __func__);

    deref_ptr_in_call: Dereferencing pointer sdp.

3916        o_count = atomic_dec_return(&sdp->open_cnt);
3917        SG_LOG(3, sfp, "%s: dev o_count after=%d: sfp=0x%pK --\n",
__func__,
3918               o_count, sfp);
3919        kfree(sfp);
3920

Dereference before null check (REVERSE_INULL)
    check_after_deref: Null-checking sdp suggests that it may be null,
but it has already been dereferenced on all paths leading to the check.

3921        if (sdp) {
3922                scsi_device_put(sdp->device);
3923                kref_put(&sdp->d_ref, sg_device_destroy);
3924        }

Line 3916 dereferences pointer sdp with &sdp->open_cnt, however later on
in line 3921 sdp is being null checked.  Either the null check is
redundant if sdp is never null or there is a potential null pointer
dereference on line 3916.

Colin