From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ABAC9C433DB for ; Mon, 15 Mar 2021 19:06:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6622664E27 for ; Mon, 15 Mar 2021 19:06:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231366AbhCOTF3 (ORCPT ); Mon, 15 Mar 2021 15:05:29 -0400 Received: from zeniv-ca.linux.org.uk ([142.44.231.140]:47720 "EHLO zeniv-ca.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232311AbhCOTE7 (ORCPT ); Mon, 15 Mar 2021 15:04:59 -0400 X-Greylist: delayed 1501 seconds by postgrey-1.27 at vger.kernel.org; Mon, 15 Mar 2021 15:04:59 EDT Received: from viro by zeniv-ca.linux.org.uk with local (Exim 4.94 #2 (Red Hat Linux)) id 1lLs1e-006J21-Od; Mon, 15 Mar 2021 18:33:10 +0000 Date: Mon, 15 Mar 2021 18:33:10 +0000 From: Al Viro To: Kees Cook Cc: Andrew Morton , Greg Kroah-Hartman , Michal Hocko , Alexey Dobriyan , Lee Duncan , Chris Leech , Adam Nichols , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH v2] seq_file: Unconditionally use vmalloc for buffer Message-ID: References: <20210315174851.622228-1-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210315174851.622228-1-keescook@chromium.org> Sender: Al Viro Precedence: bulk List-ID: X-Mailing-List: linux-hardening@vger.kernel.org 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?