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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 60B46ECAAA1 for ; Fri, 2 Sep 2022 23:08:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229504AbiIBXIL (ORCPT ); Fri, 2 Sep 2022 19:08:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53170 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229484AbiIBXIF (ORCPT ); Fri, 2 Sep 2022 19:08:05 -0400 Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A124FF1B5D for ; Fri, 2 Sep 2022 16:08:04 -0700 (PDT) Received: by mail-pg1-x52f.google.com with SMTP id x80so3213470pgx.0 for ; Fri, 02 Sep 2022 16:08:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=2Y8DDYCQ2otafS/ANml07S73pAs/kgkgSqxooJ7MzEo=; b=gZymZsu4nzF/sx2Kz8zdZO31rMaivj6sqqslSfIi8ycb86aOj28XmAjpjIbdkCFQ7r 1skyr/j6bBsNu89R3L78IRSsABgw3ZrAO6Vc19tV0nU8nt2luO9ZtwPs5x/v28CNEn/2 k4161ORZTExXAnMbY7/dBGXpFV6cKkAhVn9Hw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=2Y8DDYCQ2otafS/ANml07S73pAs/kgkgSqxooJ7MzEo=; b=MaUBoU3N15H4XRA+LZalvoGUrZFgAc3YyYsL8JhyklduDMO7k25LGVBMd6z35Rfyon SsN5LuAi40vO8C7qFdJ8HLqyA9J2JoRvuPHlxvXIGIQxSGXiep/VLsLNQT0f/qo+ohLr neDEByZg8bl+7A2ITYV59bBGyrv1pCx5zGli2VG+stGAsmsBA7GgWQkviXxGyDps3DR6 IKlmlwyxgDyI7h+oiBx2yjmB9ZDLpOKxZQ2F+HhXKYdENhOyg6LvXbF2Ptts517EtV0G VTfLsnrGQg0mqn0cP7O19g8w5YzxiCxmQoh2zA7lHN/UIoNo2eO7mlfhdOOp2qS2LXFD U75A== X-Gm-Message-State: ACgBeo04Ber/3qYD47kryO9Wl6TPzblV07NT9TK/qOd5GK8wz0mrpRPx S6pWIUTTtUpn0o/832807UjXxg== X-Google-Smtp-Source: AA6agR6G3F1xMyBZPBMpPkGlBoDBrXr4WjKwOB3BRE0dZXW8W2Nl66PYsdn9DsbS0atfylWEgULAbw== X-Received: by 2002:a63:41c5:0:b0:42c:6b7f:6d95 with SMTP id o188-20020a6341c5000000b0042c6b7f6d95mr19124355pga.175.1662160084174; Fri, 02 Sep 2022 16:08:04 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id q23-20020a170902bd9700b0016dbaf3ff2esm2207368pls.22.2022.09.02.16.08.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Sep 2022 16:08:03 -0700 (PDT) Date: Fri, 2 Sep 2022 16:08:01 -0700 From: Kees Cook To: Jakub Kicinski Cc: Pablo Neira Ayuso , Jozsef Kadlecsik , Florian Westphal , "David S. Miller" , Eric Dumazet , Paolo Abeni , syzbot , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, Harshit Mogalapalli , linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH v3] netlink: Bounds-check struct nlmsgerr creation Message-ID: <202209021555.9EE2FBD3A@keescook> References: <20220901071336.1418572-1-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220901071336.1418572-1-keescook@chromium.org> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 01, 2022 at 12:13:36AM -0700, Kees Cook wrote: > For 32-bit systems, it might be possible to wrap lnmsgerr content > lengths beyond SIZE_MAX. Explicitly test for all overflows, and mark the > memcpy() as being unable to internally diagnose overflows. > > This also excludes netlink from the coming runtime bounds check on > memcpy(), since it's an unusual case of open-coded sizing and > allocation. Avoid this future run-time warning: > > memcpy: detected field-spanning write (size 32) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16) To get rid of the above warning... > [...] > - memcpy(&errmsg->msg, nlh, nlh->nlmsg_len); > + unsafe_memcpy(&errmsg->msg, nlh, nlh->nlmsg_len, > + /* "payload" was explicitly bounds-checked, based on > + * the size of nlh->nlmsg_len. > + */); above is the "fix", since the compiler has no way to know how to bounds check the arguments. But, to write that comment, I added all these things: > [...] > - if (extack->cookie_len) > - tlvlen += nla_total_size(extack->cookie_len); > + if (extack->_msg && > + check_add_overflow(*tlvlen, nla_total_size(strlen(extack->_msg) + 1), tlvlen)) > + return false; If that's not desirable, then I guess the question I want to ask is "what can I put in the unsafe_memcpy() comment above that proves these values have been sanity checked? In other words, how do we know that tlvlen hasn't overflowed? (I don't know what other sanity checking may have already happened, so I'm looking directly at the size calculations here.) I assume this isn't more desirable: - if (extack->cookie_len) - tlvlen += nla_total_size(extack->cookie_len); + if (extack->cookie_len) { + size_t len = nla_total_size(extack->cookie_len); + + if (WARN_ON_ONCE(len > SIZE_MAX - tlvlen)) + return 0; + tlvlen += len; + } Or maybe wrap it nicely with a local macro and return 0 instead of trying to pass an error up a layer? +#define TLVADD(amount) do { \ + if (WARN_ON_ONCE(check_add_overflow(tlvlen, amount, &tlvlen))) \ + return 0; \ +} while (0) ... if (extack->cookie_len) - tlvlen += nla_total_size(extack->cookie_len); + TLVADD(nla_total_size(extack->cookie_len)); -- Kees Cook