linux-kernel - Re: [RFC] coccicheck: add a test for repeat memory fetches

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5j+Nk9r7K=FJe4v5b8mX_Fn411JMGks2sueyOFS2aTo1Ow@mail.gmail.com>
Date:   Tue, 10 Jan 2017 11:23:11 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     cocci@...teme.lip6.fr, Pengfei Wang <wpengfeinudt@...il.com>,
        Vaishali Thakkar <vthakkar1994@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] coccicheck: add a test for repeat memory fetches

On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.lawall@...6.fr> wrote:
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2159
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2257
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2302
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2342
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2365
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2406
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2439
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2491
>
> Do you want the above results?  They have the form:
>
> if (copy_from_user(&t, useraddr, sizeof(t)))
>
> My reasoning was that there could be no problem here, because the size is
> the size of the destination structure.  It doesn't depend on user level data.

They're likely false positives, but it does follow the pattern of
reading the same userspace location twice:

        if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
                return -EFAULT;

        switch (cmd) {
        case CHELSIO_SET_QSET_PARAMS:{
                int i;
                struct qset_params *q;
                struct ch_qset_params t;
                int q1 = pi->first_qset;
                int nqsets = pi->nqsets;

                if (!capable(CAP_NET_ADMIN))
                        return -EPERM;
                if (copy_from_user(&t, useraddr, sizeof(t)))
                        return -EFAULT;

If there is any logic that examines cmd (u32) and operates on t
(struct ch_qset_params), there could be a flaw. It doesn't look like
it here, but a "correct" version of this would be:

                if (copy_from_user(&t, useraddr, sizeof(t)))
                        return -EFAULT;
                t.cmd = cmd;


-Kees

-- 
Kees Cook
Nexus Security