From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424887AbdD1QSy (ORCPT ); Fri, 28 Apr 2017 12:18:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42294 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S968250AbdD1QSn (ORCPT ); Fri, 28 Apr 2017 12:18:43 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 31363C059747 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=queasysnail.net Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=none smtp.mailfrom=sd@queasysnail.net DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 31363C059747 Date: Fri, 28 Apr 2017 18:18:40 +0200 From: Sabrina Dubroca To: "Jason A. Donenfeld" Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, David.Laight@aculab.com, kernel-hardening@lists.openwall.com, davem@davemloft.net Subject: Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Message-ID: <20170428161840.GA30423@bistromath.localdomain> References: <20170425155215.4835-1-Jason@zx2c4.com> <20170425184734.26563-1-Jason@zx2c4.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170425184734.26563-1-Jason@zx2c4.com> User-Agent: Mutt/1.8.2 (2017-04-18) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 28 Apr 2017 16:18:43 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-04-25, 20:47:30 +0200, Jason A. Donenfeld wrote: > This is a defense-in-depth measure in response to bugs like > 4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec"). While > we're at it, we also limit the amount of recursion this function is > allowed to do. Not actually providing a bounded base case is a future > diaster that we can easily avoid here. > > Signed-off-by: Jason A. Donenfeld > --- > Changes v5->v6: > * Use unlikely() for the rare overflow conditions. > * Also bound recursion, since this is a potential disaster we can avert. > > net/core/skbuff.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index f86bf69cfb8d..24fb53f8534e 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -3489,16 +3489,22 @@ void __init skb_init(void) > * @len: Length of buffer space to be mapped > * > * Fill the specified scatter-gather list with mappings/pointers into a > - * region of the buffer space attached to a socket buffer. > + * region of the buffer space attached to a socket buffer. Returns either > + * the number of scatterlist items used, or -EMSGSIZE if the contents > + * could not fit. > */ One small thing here: since you're touching this comment, could you move it next to skb_to_sgvec, since that's the function it's supposed to document? Thanks! > static int > -__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len) > +__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len, > + unsigned int recursion_level) -- Sabrina