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?
