[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YE+oZkSVNyaONMd9@zeniv-ca.linux.org.uk>
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?
Powered by blists - more mailing lists