From: Andy Lutomirski Subject: Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5) Date: Thu, 22 Dec 2016 11:56:34 -0800 Message-ID: References: <1482425969.2673.5.camel@stressinduktion.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Hannes Frederic Sowa , Daniel Borkmann , "Jason A. Donenfeld" , "kernel-hardening@lists.openwall.com" , "Theodore Ts'o" , Netdev , LKML , Linux Crypto Mailing List , David Laight , Eric Dumazet , Linus Torvalds , Eric Biggers , Tom Herbert , Andi Kleen , "David S. Miller" , Jean-Philippe Aumasson To: Alexei Starovoitov Return-path: Received: from mail.kernel.org ([198.145.29.136]:38940 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966744AbcLVT5B (ORCPT ); Thu, 22 Dec 2016 14:57:01 -0500 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A04312041B for ; Thu, 22 Dec 2016 19:56:59 +0000 (UTC) Received: from mail-ua0-f173.google.com (mail-ua0-f173.google.com [209.85.217.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 1374D203F3 for ; Thu, 22 Dec 2016 19:56:56 +0000 (UTC) Received: by mail-ua0-f173.google.com with SMTP id 34so55362417uac.1 for ; Thu, 22 Dec 2016 11:56:56 -0800 (PST) In-Reply-To: Sender: linux-crypto-owner@vger.kernel.org List-ID: On Thu, Dec 22, 2016 at 11:34 AM, Alexei Starovoitov wrote: > On Thu, Dec 22, 2016 at 9:25 AM, Andy Lutomirski wrote: >> On Thu, Dec 22, 2016 at 8:59 AM, Hannes Frederic Sowa >> wrote: >>> On Thu, 2016-12-22 at 08:07 -0800, Andy Lutomirski wrote: >>> >>> We don't prevent ebpf programs being loaded based on the digest but >>> just to uniquely identify loaded programs from user space and match up >>> with their source. >> >> The commit log talks about using the hash to see if the program has >> already been compiled and JITted. If that's done, then a collision >> will directly cause the kernel to malfunction. > > Andy, please read the code. > we could have used jhash there just as well. > Collisions are fine. There's relevant in the code to read yet AFAICS. The code exports it via fdinfo, and userspace is expected to do something with it. The commit message says: When programs are pinned and retrieved by an ELF loader, the loader can check the program's digest through fdinfo and compare it against one that was generated over the ELF file's program section to see if the program needs to be reloaded. I assume this means that a userspace component is expected to compare the digest of a loaded program to a digest of a program it wants to load and to use the result of the comparison to decide whether the programs are the same. If that's indeed the case (and it sure sounds like it, and I fully expect CRIU to do very similar things when support is added), then malicious collisions do matter. It's also not quite clear to me why userspace needs to be able to calculate the digest on its own. A bpf(BPF_CALC_PROGRAM_DIGEST) command that takes a BPF program as input and hashes it would seem to serve the same purpose, and that would allow the kernel to key the digest and change the algorithm down the road without breaking things. Regardless, adding a new hash algorithm that is almost-but-not-quite SHA-1 and making it a stable interface to userspace is not a good thing. --Andy