From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:48668 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751881AbeA2Mye (ORCPT ); Mon, 29 Jan 2018 07:54:34 -0500 Subject: Re: [PATCH] btrfs: Fix UAF To: Nikolay Borisov , linux-btrfs@vger.kernel.org Cc: dsterba@suse.cz References: <1516972827-1481-1-git-send-email-nborisov@suse.com> <8c70d5d1-16b7-d1a0-b2b1-6282ee12f0b0@oracle.com> <92e89204-af54-e38f-4a0d-e73b70280cbf@suse.com> From: Anand Jain Message-ID: Date: Mon, 29 Jan 2018 20:55:44 +0800 MIME-Version: 1.0 In-Reply-To: <92e89204-af54-e38f-4a0d-e73b70280cbf@suse.com> Content-Type: text/plain; charset=utf-8; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 01/29/2018 03:01 PM, Nikolay Borisov wrote: > > > On 29.01.2018 04:38, Anand Jain wrote: >> >> >> On 01/26/2018 09:20 PM, Nikolay Borisov wrote: >>> Commit 4fde46f0cc71 ("Btrfs: free the stale device") introduced >>> btrfs_free_stale_device which iterates the device lists for all >>> registered btrfs filesystems and deletes those devices which aren't >>> mounted. In a btrfs_devices structure has only 1 device attached to it >>> and it is unused then btrfs_free_stale_devices will proceed to also >>> free the btrfs_fs_devices struct itself. Currently this leads to a UAF >>> since list_for_each_entry will try to perform a check on the already- >>> freed memory to see if it has to terminated the loop. >>> >>> The fix is to use 'break' when we know we are freeing the current >>> fs_devs. >> >>  No break is needed as we need to iterate all stale devices and delete >>  the found stale entry, so commit [1] used list_for_each_entry_safe() >>  and removed the break, > > We only do the break if we know we have a single device in the current > fs_devs struct. And executing free_fs_devices would have already freed > the device + fs_devs. > >> >>  [1] >>   commit 38cf665d338fca33af4b16f9ec7cad6637fc0fec >>   Author: Anand Jain >>     btrfs: make btrfs_free_stale_device() to iterate all stales >> >> >>  I am guessing UAF might be in[2], instead ? >> >>  [2] >>     free_fs_devices(fs_devs) >> :: >>         while (!list_empty(&fs_devices->devices)) { >>                 device = list_entry(fs_devices->devices.next, >>                                     struct btrfs_device, dev_list); > > It's not that, I thought so at first. But here using list_empty you are > awlays accessing the head the of the list, which is guaranteed to be > valid since you do the freeing of fs_devices outside of the while loop. break when num_devices == 1 is fine; Reviewed-by: Anand Jain Thanks, Anand >> >> Thanks, Anand >> >>> Fixes: 4fde46f0cc71 ("Btrfs: free the stale device") >>> Signed-off-by: Nikolay Borisov >>> >>> --- >>>   fs/btrfs/volumes.c | 1 + >>>   1 file changed, 1 insertion(+) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index f7147740b68e..c3ab55336ee0 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -645,6 +645,7 @@ static void btrfs_free_stale_devices(const char >>> *path, >>>                   btrfs_sysfs_remove_fsid(fs_devs); >>>                   list_del(&fs_devs->list); >>>                   free_fs_devices(fs_devs); >>> +                break; >>>               } else { >>>                   fs_devs->num_devices--; >>>                   list_del(&dev->dev_list); >>> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >