Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756456AbdDRLe0 (ORCPT ); Tue, 18 Apr 2017 07:34:26 -0400 Received: from a.mx.secunet.com ([62.96.220.36]:50088 "EHLO a.mx.secunet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751228AbdDRLeX (ORCPT ); Tue, 18 Apr 2017 07:34:23 -0400 Date: Tue, 18 Apr 2017 13:34:12 +0200 From: Steffen Klassert To: Herbert Xu CC: Cong Wang , Andrey Konovalov , "David S. Miller" , netdev , LKML , Dmitry Vyukov , Kostya Serebryany , syzkaller , Eric Dumazet Subject: Re: ney/key: slab-out-of-bounds in parse_ipsecrequests Message-ID: <20170418113412.GA15917@secunet.com> References: <20170413103559.GA23263@gondor.apana.org.au> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20170413103559.GA23263@gondor.apana.org.au> User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [10.182.7.193] X-G-Data-MailSecurity-for-Exchange-State: 0 X-G-Data-MailSecurity-for-Exchange-Error: 0 X-G-Data-MailSecurity-for-Exchange-Sender: 23 X-G-Data-MailSecurity-for-Exchange-Server: d65e63f7-5c15-413f-8f63-c0d707471c93 X-EXCLAIMER-MD-CONFIG: 2c86f778-e09b-4440-8b15-867914633a10 X-G-Data-MailSecurity-for-Exchange-Guid: 006D5D18-D90B-454E-B9B3-08C33ABC7A89 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2591 Lines: 63 On Thu, Apr 13, 2017 at 06:35:59PM +0800, Herbert Xu wrote: > On Wed, Apr 12, 2017 at 05:39:22PM -0700, Cong Wang wrote: > > On Wed, Apr 12, 2017 at 8:02 AM, Andrey Konovalov wrote: > > > Hi, > > > > > > I've got the following error report while fuzzing the kernel with syzkaller. > > > > > > On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6). > > > > > > A reproducer and .config are attached. > > > > > > When subtracting rq->sadb_x_ipsecrequest_len from len it can become > > > negative and the while loop condition remains true. > > > > Good catch! Seems the fix is pretty straight forward: > > > > diff --git a/net/key/af_key.c b/net/key/af_key.c > > index c6252ed..cbce595 100644 > > --- a/net/key/af_key.c > > +++ b/net/key/af_key.c > > @@ -1945,7 +1945,7 @@ parse_ipsecrequests(struct xfrm_policy *xp, > > struct sadb_x_policy *pol) > > if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy)) > > return -EINVAL; > > > > - while (len >= sizeof(struct sadb_x_ipsecrequest)) { > > + while (len >= (int)sizeof(struct sadb_x_ipsecrequest)) { > > if ((err = parse_ipsecrequest(xp, rq)) < 0) > > return err; > > len -= rq->sadb_x_ipsecrequest_len; > > > > But pol->sadb_x_policy_len and rq->sadb_x_ipsecrequest_len > > are controllable by user (fortunately root), I am feeling there might > > be other problem I miss too. > > Well the fact that it is negative means that you've already parsed > crap in the previous loop. This interface really needs to die. Do you see a chance to remove this? I guess it is not used frequently anymore, but distros still ship the old ipsec tools. > > ---8<--- > Subject: af_key: Fix sadb_x_ipsecrequest parsing > > The parsing of sadb_x_ipsecrequest is broken in a number of ways. > First of all we're not verifying sadb_x_ipsecrequest_len. This > is needed when the structure carries addresses at the end. Worse > we don't even look at the length when we parse those optional > addresses. > > The migration code had similar parsing code that's better but > it also has some deficiencies. The length is overcounted first > of all as it includes the header itself. It also fails to check > the length before dereferencing the sa_family field. > > This patch fixes those problems in parse_sockaddr_pair and then > uses it in parse_ipsecrequest. > > Reported-by: Andrey Konovalov > Signed-off-by: Herbert Xu Applied to the ipsec tree, thanks Herbert!