From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:52306 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727033AbeHKDVE (ORCPT ); Fri, 10 Aug 2018 23:21:04 -0400 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w7B0jLRq141103 for ; Sat, 11 Aug 2018 00:48:50 GMT Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by userp2120.oracle.com with ESMTP id 2kn4sq97v0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Sat, 11 Aug 2018 00:48:49 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w7B0mnjt008534 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Sat, 11 Aug 2018 00:48:49 GMT Received: from abhmp0017.oracle.com (abhmp0017.oracle.com [141.146.116.23]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w7B0mnfi008006 for ; Sat, 11 Aug 2018 00:48:49 GMT Date: Fri, 10 Aug 2018 17:48:47 -0700 From: "Darrick J. Wong" Subject: Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree Message-ID: <20180811004847.GB11750@magnolia> References: <1524017385-6671-1-git-send-email-shan.hai@oracle.com> <9fbccc81-103f-5b21-af3c-e32b82eba24b@oracle.com> <20180418022426.GL24738@magnolia> <49bd78f6-8c4d-c51a-6fa1-02529f0da7ca@oracle.com> <8a1826e7-d58c-352a-74ad-be9486882704@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8a1826e7-d58c-352a-74ad-be9486882704@oracle.com> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Shan Hai Cc: linux-xfs@vger.kernel.org On Thu, Apr 19, 2018 at 09:18:38AM +0800, Shan Hai wrote: > Hi Darrick, > > > On 2018年04月18日 10:41, Shan Hai wrote: > > > > > > On 2018年04月18日 10:24, Darrick J. Wong wrote: > > > On Wed, Apr 18, 2018 at 10:13:22AM +0800, Shan Hai wrote: > > > > > > > > On 2018年04月18日 10:09, Shan Hai wrote: > > > > > Fuzzing tool reports a write to null pointer error in the > > > > > xfs_bmap_extents_to_btree, fix it by bailing out on encountering > > > > > a null pointer. > > > > > > > > > > Signed-off-by: Shan Hai > > > > This one supposed to be applied on top of below: > > > > > > > > https://www.spinics.net/lists/linux-xfs/msg17254.html > > > > [PATCH] xfs: set format back to extents if > > > > xfs_bmap_extents_to_btree fails > > > > > > > > Thanks > > > > Shan Hai > > > > > --- > > > > >   fs/xfs/libxfs/xfs_bmap.c | 24 ++++++++++++++++-------- > > > > >   1 file changed, 16 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > > > > > index 040eeda..90b743d 100644 > > > > > --- a/fs/xfs/libxfs/xfs_bmap.c > > > > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > > > > @@ -724,19 +724,14 @@ xfs_bmap_extents_to_btree( > > > > >       args.wasdel = wasdel; > > > > >       *logflagsp = 0; > > > > >       if ((error = xfs_alloc_vextent(&args))) { > > > > > -        xfs_iroot_realloc(ip, -1, whichfork); > > > > >           ASSERT(ifp->if_broot == NULL); > > > > > -        XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > > > > > -        xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); > > > > > -        return error; > > > > > +        goto err1; > > > > >       } > > > > >       if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) { > > > > > -        xfs_iroot_realloc(ip, -1, whichfork); > > > > >           ASSERT(ifp->if_broot == NULL); > > > > > -        XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > > > > > -        xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); > > > > > -        return -ENOSPC; > > > > > +        error = -ENOSPC; > > > > > +        goto err1; > > > > >       } > > > > >       /* > > > > >        * Allocation can't fail, the space was reserved. > > > > > @@ -748,6 +743,10 @@ xfs_bmap_extents_to_btree( > > > > >       ip->i_d.di_nblocks++; > > > > >       xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L); > > > > >       abp = xfs_btree_get_bufl(mp, tp, args.fsbno, 0); > > > > > +    if (!abp) { > > > When does this happen?  We got args.fsbno from the allocator, so we're > > > not out of space.  Or are you saying that something fuzzed the free > > > space btree, therefore the allocator gave back a nonsense block number > > > (say pointing past the end of the fs), and therefore the _get_bufl call > > > returned NULL? > > > > Seems the memory  page allocation fails and the xfs_btree_get_bufl > > returns NULL, but I have no reliable reproducer, sorry. > > > > I have got another report of a possible NULL pointer deference in the code > as below, > this time from a static code analysis tool: > xfs_bmap_extents_to_btree >   ->abp = xfs_btree_get_bufl >     ->return xfs_trans_get_buf >       ->return xfs_trans_get_buf_map >         ->bp = xfs_buf_get_map >             if (bp == NULL) { >                 return NULL; >         } > > As you know that the corrupted block numbers from block allocator is checked > in the _xfs_buf_find, which returns NULL as well after a WARN_ON if the the > bno is > invalid. > > And the maximum allocation size in the xfs_buf_get_map is a single page, so > because of the PAGE_ALLOC_COSTLY_ORDER logic it's highly possible that the > process is killed the OOM killer instead of returning NULL from both > kmalloc/alloc_page. > > Even though this NULL pointer problem is rarely seen in the real workload > but it's > logically correct to check the pointer validity as this patch does in my > opinion. Er... sorry for the four month delay. Looks reasonable, will test... Reviewed-by: Darrick J. Wong --D > Thanks > Shan Hai > > Thanks > > Shan Hai > > > If so, maybe we need to change that WARN_ON_ONCE thing above to: > > > > > > if (WARN_ON_ONCE(...) || !xfs_verify_fsbno(..., args.fsbno)) { > > >     /* undo state and return */ > > > } > > > > > > --D > > > > > > > > +        error = -ENOSPC; > > > > > +        goto err2; > > > > > +    } > > > > >       /* > > > > >        * Fill in the child block. > > > > >        */ > > > > > @@ -787,6 +786,15 @@ xfs_bmap_extents_to_btree( > > > > >       *curp = cur; > > > > >       *logflagsp = XFS_ILOG_CORE | xfs_ilog_fbroot(whichfork); > > > > >       return 0; > > > > > + > > > > > +err2: > > > > > +    xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L); > > > > > +err1: > > > > > +    xfs_iroot_realloc(ip, -1, whichfork); > > > > > +    XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > > > > > +    xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); > > > > > + > > > > > +    return error; > > > > >   } > > > > >   /* > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at  http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html