From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f175.google.com (mail-pl1-f175.google.com [209.85.214.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 97BBF20E8 for ; Wed, 4 May 2022 03:37:37 +0000 (UTC) Received: by mail-pl1-f175.google.com with SMTP id n8so337696plh.1 for ; Tue, 03 May 2022 20:37:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=lsBVjxx2frcPFNjY4/BLqdzh7cys7mpO3LyMH5LAbSE=; b=e/seADFb6dKCy5O35I2lb3QkGM1U1AG+HWJjIUEuWUaLYiQ/bRCqVwsL3BCOvWYEuK 9qpY1aU7RmqPGGBT5JUv2H6Jc6siGFaT+PVsx+FN+n4iHis5NM0gOyiK9HY2IIWzLhh4 mk3V/alwvU0/DDln8pB7kZALicerW3KUceFLU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=lsBVjxx2frcPFNjY4/BLqdzh7cys7mpO3LyMH5LAbSE=; b=f89wvRTmaSIdSPqwMOYfLwM2roywuUOjr0hmcbHFac7SGnhK/vDKEiZzgUIQUnEoGT mk1zFUphJkVCjhzRxphLiCfy4Fsy3hh7jKQUUNvXeAwqvJ3W7I5zpBoAeNw493Fyd0xz eWV4OjyydVu3g2LAdNZMgDXLah+niyCekcRdm4eJTG9wklBMCq/zv4ZSFQShUAorPHqE UkYND+BCLwR+P4jvYmHIu9ED1Qh0sD6+B2ZvLC2iRlX+7h7HuDkv9zfkYnMVgYmxlMrt HnTmc3bwt8FWlz6DoLXmd/UvZ/46irw1Sj09ljKf/OUYaKEJJoi6mBDQ2f/od0t1PkWa QJMg== X-Gm-Message-State: AOAM53148svz+Q/97l6z8LjTCCpGj27WUKy4XmjWfZ5ETK4MtXC8rR6k hSJhsAtWThBOMQPqDcTIFUsptw== X-Google-Smtp-Source: ABdhPJw42Y4zw7XGLJ0qL8m2iK8R/sg3Y/yQJpglfujQWmDGz21UKIQOVg7Md/ZATtyrGT1c7jNF4A== X-Received: by 2002:a17:90b:1b44:b0:1dc:315f:4510 with SMTP id nv4-20020a17090b1b4400b001dc315f4510mr8282445pjb.28.1651635456924; Tue, 03 May 2022 20:37:36 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id k23-20020a170902ba9700b0015e8d4eb1fesm7117388pls.72.2022.05.03.20.37.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 May 2022 20:37:36 -0700 (PDT) Date: Tue, 3 May 2022 20:37:35 -0700 From: Kees Cook To: "Gustavo A. R. Silva" Cc: Rasmus Villemoes , "David S. Miller" , Jakub Kicinski , Rich Felker , Eric Dumazet , netdev@vger.kernel.org, Alexei Starovoitov , alsa-devel@alsa-project.org, Al Viro , Andrew Gabbasov , Andrew Morton , Andy Gross , Andy Lavr , Arend van Spriel , Baowen Zheng , Bjorn Andersson , Boris Ostrovsky , Bradley Grove , brcm80211-dev-list.pdl@broadcom.com, Christian Brauner , Christian =?iso-8859-1?Q?G=F6ttsche?= , Christian Lamparter , Chris Zankel , Cong Wang , Daniel Axtens , Daniel Vetter , Dan Williams , David Gow , David Howells , Dennis Dalessandro , devicetree@vger.kernel.org, Dexuan Cui , Dmitry Kasatkin , Eli Cohen , Eric Paris , Eugeniu Rosca , Felipe Balbi , Francis Laniel , Frank Rowand , Franky Lin , Greg Kroah-Hartman , Gregory Greenman , Guenter Roeck , Haiyang Zhang , Hante Meuleman , Herbert Xu , Hulk Robot , "James E.J. Bottomley" , James Morris , Jarkko Sakkinen , Jaroslav Kysela , Jason Gunthorpe , Jens Axboe , Johan Hedberg , Johannes Berg , Johannes Berg , John Keeping , Juergen Gross , Kalle Valo , Keith Packard , keyrings@vger.kernel.org, kunit-dev@googlegroups.com, Kuniyuki Iwashima , "K. Y. Srinivasan" , Lars-Peter Clausen , Lee Jones , Leon Romanovsky , Liam Girdwood , linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-hardening@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-integrity@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-xtensa@linux-xtensa.org, llvm@lists.linux.dev, Loic Poulain , Louis Peens , Luca Coelho , Luiz Augusto von Dentz , Marc Dionne , Marcel Holtmann , Mark Brown , "Martin K. Petersen" , Max Filippov , Mimi Zohar , Muchun Song , Nathan Chancellor , Nick Desaulniers , Nuno =?iso-8859-1?Q?S=E1?= , Paolo Abeni , Paul Moore , Rob Herring , Russell King , selinux@vger.kernel.org, "Serge E. Hallyn" , SHA-cyfmac-dev-list@infineon.com, Simon Horman , Stefano Stabellini , Stefan Richter , Steffen Klassert , Stephen Hemminger , Stephen Smalley , Tadeusz Struk , Takashi Iwai , Tom Rix , Udipto Goswami , Vincenzo Frascino , wcn36xx@lists.infradead.org, Wei Liu , xen-devel@lists.xenproject.org, Xiu Jianfeng , Yang Yingliang Subject: Re: [PATCH 01/32] netlink: Avoid memcpy() across flexible array boundary Message-ID: <202205032027.B2A9FB4AA@keescook> References: <20220504014440.3697851-1-keescook@chromium.org> <20220504014440.3697851-2-keescook@chromium.org> <20220504033105.GA13667@embeddedor> Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220504033105.GA13667@embeddedor> On Tue, May 03, 2022 at 10:31:05PM -0500, Gustavo A. R. Silva wrote: > On Tue, May 03, 2022 at 06:44:10PM -0700, Kees Cook wrote: > [...] > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > index 1b5a9c2e1c29..09346aee1022 100644 > > --- a/net/netlink/af_netlink.c > > +++ b/net/netlink/af_netlink.c > > @@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err, > > NLMSG_ERROR, payload, flags); > > errmsg = nlmsg_data(rep); > > errmsg->error = err; > > - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); > > + errmsg->msg = *nlh; > > + if (payload > sizeof(*errmsg)) > > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, > > + nlh->nlmsg_len - sizeof(*nlh)); > > They have nlmsg_len()[1] for the length of the payload without the header: > > /** > * nlmsg_len - length of message payload > * @nlh: netlink message header > */ > static inline int nlmsg_len(const struct nlmsghdr *nlh) > { > return nlh->nlmsg_len - NLMSG_HDRLEN; > } Oh, hm, yeah, that would be much cleaner. The relationship between "payload" and nlmsg_len is confusing in here. :) So, this should be simpler: - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); + errmsg->msg = *nlh; + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh)); It's actually this case that triggered my investigation in __bos(1)'s misbehavior around sub-structs, since this case wasn't getting silenced: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832 It still feels like it should be possible to get this right without splitting the memcpy, though. Hmpf. > (would that function use some sanitization, though? what if nlmsg_len is > somehow manipulated to be less than NLMSG_HDRLEN?...) Maybe something like: static inline int nlmsg_len(const struct nlmsghdr *nlh) { if (WARN_ON(nlh->nlmsg_len < NLMSG_HDRLEN)) return 0; return nlh->nlmsg_len - NLMSG_HDRLEN; } > Also, it seems there is at least one more instance of this same issue: > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > index 16ae92054baa..d06184b94af5 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -1723,7 +1723,8 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb, > nlh->nlmsg_seq, NLMSG_ERROR, payload, 0); > errmsg = nlmsg_data(rep); > errmsg->error = ret; > - memcpy(&errmsg->msg, nlh, nlh->nlmsg_len); > + errmsg->msg = *nlh; > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh)); Ah, yes, nice catch! > cmdattr = (void *)&errmsg->msg + min_len; > > ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr, > > -- > Gustavo > > [1] https://elixir.bootlin.com/linux/v5.18-rc5/source/include/net/netlink.h#L577 -- Kees Cook