linux-hardening - Re: [PATCH v2] seq_file: Unconditionally use vmalloc for buffer

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]

Date:   Mon, 15 Mar 2021 18:33:10 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Kees Cook <keescook@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michal Hocko <mhocko@...e.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Lee Duncan <lduncan@...e.com>, Chris Leech <cleech@...hat.com>,
        Adam Nichols <adam@...mm-co.com>, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] seq_file: Unconditionally use vmalloc for buffer

On Mon, Mar 15, 2021 at 10:48:51AM -0700, Kees Cook wrote:
> The sysfs interface to seq_file continues to be rather fragile, as seen
> with some recent exploits[1]. Move the seq_file buffer to the vmap area
> (while retaining the accounting flag), since it has guard pages that
> will catch and stop linear overflows. This seems justified given that
> seq_file already uses kvmalloc(), is almost always using a PAGE_SIZE or
> larger allocation, has allocations are normally short lived, and is not
> normally on a performance critical path.

You are attacking the wrong part of it.  Is there any reason for having
seq_get_buf() public in the first place?

For example, the use in blkcg_print_stat() is entirely due to the bogus
->pd_stat_fn() calling conventions.  Fuck scnprintf() games, just pass
seq_file to ->pd_stat_fn() and use seq_printf() instead.  Voila - no
seq_get_buf()/seq_commit()/scnprintf() garbage.

tegra use is no better, AFAICS.  inifinibarf one...  allow me to quote
that gem in full:
static int _driver_stats_seq_show(struct seq_file *s, void *v)
{
        loff_t *spos = v;
        char *buffer;
        u64 *stats = (u64 *)&hfi1_stats;
        size_t sz = seq_get_buf(s, &buffer);

        if (sz < sizeof(u64))
                return SEQ_SKIP;
        /* special case for interrupts */
        if (*spos == 0)
                *(u64 *)buffer = hfi1_sps_ints();
        else
                *(u64 *)buffer = stats[*spos];
        seq_commit(s,  sizeof(u64));
        return 0;
}
Yes, really.  Not to mention that there's seq_write(), what the _hell_
is it using seq_file for in the first place?  Oh, and hfi_stats is
actually this:
struct hfi1_ib_stats {
        __u64 sps_ints; /* number of interrupts handled */
        __u64 sps_errints; /* number of error interrupts */
        __u64 sps_txerrs; /* tx-related packet errors */
        __u64 sps_rcverrs; /* non-crc rcv packet errors */
        __u64 sps_hwerrs; /* hardware errors reported (parity, etc.) */
        __u64 sps_nopiobufs; /* no pio bufs avail from kernel */
        __u64 sps_ctxts; /* number of contexts currently open */
        __u64 sps_lenerrs; /* number of kernel packets where RHF != LRH len */
        __u64 sps_buffull;
        __u64 sps_hdrfull;
};
I won't go into further details - CDA might be dead and buried, but there
should be some limit to public obscenity ;-/

procfs use is borderline - it looks like there might be a good cause
for seq_escape_str().

And sysfs_kf_seq_show()...  Do we want to go through seq_file there at
all?