linux-kernel - re: mm: brk: downgrade mmap

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:   Thu, 4 Oct 2018 19:08:13 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Yang Shi <yang.shi@...ux.alibaba.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>
Cc:     linux-mm@...ck.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: mm: brk: downgrade mmap_sem to read when shrinking

Hi,

Static analysis has found a couple of issues as follows:

commit 551f205aff9198e17add1264dd781771d1a2bd9d
Author: Yang Shi <yang.shi@...ux.alibaba.com>
Date:   Thu Oct 4 07:43:18 2018 +1000

    mm: brk: downgrade mmap_sem to read when shrinking

Static analysis with CoverityScan has detected an issue in mm/mmap.c,
function do_brk_flags():

                retval = __do_munmap(mm, newbrk, oldbrk-newbrk, &uf, true);
                if (retval < 0) {
                        mm->brk = origbrk;
                        goto out;
                } else if (retval == 1)
                        downgraded = true;

retval is unsigned long, so the retval < 0 check is always false, which
looks bogus to me.

Also same kind of issue with:

commit e66477708ec2a764d3add92ca59134e3812da0bb
Author: Yang Shi <yang.shi@...ux.alibaba.com>
Date:   Thu Oct 4 07:43:18 2018 +1000

    mm: mremap: downgrade mmap_sem to read when shrinking

                ret = __do_munmap(mm, addr+new_len, old_len - new_len,
                                  &uf_unmap, true);
                if (ret < 0 && old_len != new_len)
                        goto out;
                /* Returning 1 indicates mmap_sem is downgraded to read. */
                else if (ret == 1)
                        downgraded = true;

again, ret is unsigned long, so the comparison with ret < 0 is always false.

Detected by CoverityScan, CID#1473794, CID#1473791 "Unsigned compared
against 0".

Colin