From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 89DEC1641A for ; Thu, 8 Jun 2023 16:12:23 +0000 (UTC) Received: from lithops.sigma-star.at (lithops.sigma-star.at [195.201.40.130]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8C28311A; Thu, 8 Jun 2023 09:12:19 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by lithops.sigma-star.at (Postfix) with ESMTP id 093FC605DED7; Thu, 8 Jun 2023 18:12:12 +0200 (CEST) Received: from lithops.sigma-star.at ([127.0.0.1]) by localhost (lithops.sigma-star.at [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id YsTv-yOP3HrN; Thu, 8 Jun 2023 18:12:11 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by lithops.sigma-star.at (Postfix) with ESMTP id 7E9E36081100; Thu, 8 Jun 2023 18:12:11 +0200 (CEST) Received: from lithops.sigma-star.at ([127.0.0.1]) by localhost (lithops.sigma-star.at [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id VYHkTxYt_ulp; Thu, 8 Jun 2023 18:12:11 +0200 (CEST) Received: from lithops.sigma-star.at (lithops.sigma-star.at [195.201.40.130]) by lithops.sigma-star.at (Postfix) with ESMTP id 508B4605DED7; Thu, 8 Jun 2023 18:12:11 +0200 (CEST) Date: Thu, 8 Jun 2023 18:12:11 +0200 (CEST) From: Richard Weinberger To: Petr Mladek Cc: Kees Cook , linux-hardening , netdev , linux-kernel , Steven Rostedt , senozhatsky , Andy Shevchenko , Rasmus Villemoes , davem , edumazet , kuba , pabeni , Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn?= Roy Baron , Benno Lossin , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend Message-ID: <520885538.3699431.1686240731083.JavaMail.zimbra@nod.at> In-Reply-To: References: <20230607223755.1610-1-richard@nod.at> <202306071634.51BBAFD14@keescook> Subject: Re: [RFC PATCH 0/1] Integer overflows while scanning for integers Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Originating-IP: [195.201.40.130] X-Mailer: Zimbra 8.8.12_GA_3807 (ZimbraWebClient - FF97 (Linux)/8.8.12_GA_3809) Thread-Topic: Integer overflows while scanning for integers Thread-Index: eRz4qdIczSWHayESvWhfMkCVWHkibA== X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, T_SCC_BODY_TEXT_LINE,T_SPF_PERMERROR autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net ----- Urspr=C3=BCngliche Mail ----- > Von: "Petr Mladek" > On Wed 2023-06-07 16:36:12, Kees Cook wrote: >> On Thu, Jun 08, 2023 at 12:37:54AM +0200, Richard Weinberger wrote: >> > Hi! >> >=20 >> > Lately I wondered whether users of integer scanning functions check >> > for overflows. >> > To detect such overflows around scanf I came up with the following >> > patch. It simply triggers a WARN_ON_ONCE() upon an overflow. >> >=20 >> > After digging into various scanf users I found that the network device >> > naming code can trigger an overflow. >> >=20 >> > e.g: >> > $ ip link add 1 type veth peer name 9999999999 >> > $ ip link set name "%d" dev 1 >> >=20 >> > It will trigger the following WARN_ON_ONCE(): >> > ------------[ cut here ]------------ >> > WARNING: CPU: 2 PID: 433 at lib/vsprintf.c:3701 vsscanf+0x6ce/0x990 >>=20 >> Hm, it's considered a bug if a WARN or BUG can be reached from >> userspace, >=20 > Good point. WARN() does not look like the right way in this case. Well, the whole point of my RFC(!) patch is showing the issue and providing= a way to actually find such call sites, like the netdev code. =20 > Another problem is that some users use panic_on_warn. In this case, > the above "ip" command calls would trigger panic(). And it does not > look like an optimal behavior. Only if we don't fix netdev code. =20 > I know there already are some WARN_ONs for similar situations, e.g. > set_field_width() or set_precision(). But these do not get random > values. And it would actually be nice to introduce something like > INFO() that would be usable for these less serious problems where > the backtrace is useful but they should never trigger panic(). >=20 >> so this probably needs to be rearranged (or callers fixed). >> Do we need to change the scanf API for sane use inside the kernel? >=20 > It seems that userspace implementation of sscanf() and vsscanf() > returns -ERANGE in this case. It might be a reasonable solution. >=20 > Well, there is a risk of introducing security problems. The error > value might cause an underflow/overflow when the caller does not expect > a negative value. Agreed. Without inspecting all users of scanf we cannot change the API. > Alternative solution would be to update the "ip" code so that it > reads the number separately and treat zero return value as > -EINVAL. The kernel needs fixing, not userspace. Thanks, //richard