btrfs: Prevent possible ERR_PTR() dereference

Commit Message

Nikolay Borisov July 11, 2017, 1:55 p.m. UTC

In btrfs_full_stripe_len/btrfs_is_parity_mirror we have similar code which
gets the chunk map for a particular range via get_chunk_map. However,
get_chunk_map can return an ERR_PTR value and while the 2 callers do catch
this with a WARN_ON they then proceed to indiscriminately dereference the
extent map. This of course leads to a crash. Fix the offenders by making the
dereference conditional on IS_ERR.

Signed-off-by: Nikolay Borisov <[e-mail]>
---
 fs/btrfs/volumes.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

David Sterba July 11, 2017, 5:24 p.m. UTC | #1

On Tue, Jul 11, 2017 at 04:55:51PM +0300, Nikolay Borisov wrote:
> In btrfs_full_stripe_len/btrfs_is_parity_mirror we have similar code which
> gets the chunk map for a particular range via get_chunk_map. However,
> get_chunk_map can return an ERR_PTR value and while the 2 callers do catch
> this with a WARN_ON they then proceed to indiscriminately dereference the
> extent map. This of course leads to a crash. Fix the offenders by making the
> dereference conditional on IS_ERR.

While the code makes it better, the whole callchain should be fixed. The
WARN_ON used to be a BUG_ON and the error handling was absent, and still
is. Although it's unlikely to see the warnings from that, I'd rather see
it fixed properly. The direct caller of btrfs_full_stripe_len will be
able to handle it.

> Signed-off-by: Nikolay Borisov <[e-mail]>

Reviewed-by: David Sterba <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nikolay Borisov July 11, 2017, 7:29 p.m. UTC | #2

On 11.07.2017 20:24, David Sterba wrote:
> On Tue, Jul 11, 2017 at 04:55:51PM +0300, Nikolay Borisov wrote:
>> In btrfs_full_stripe_len/btrfs_is_parity_mirror we have similar code which
>> gets the chunk map for a particular range via get_chunk_map. However,
>> get_chunk_map can return an ERR_PTR value and while the 2 callers do catch
>> this with a WARN_ON they then proceed to indiscriminately dereference the
>> extent map. This of course leads to a crash. Fix the offenders by making the
>> dereference conditional on IS_ERR.
> 
> While the code makes it better, the whole callchain should be fixed. The
> WARN_ON used to be a BUG_ON and the error handling was absent, and still
> is. Although it's unlikely to see the warnings from that, I'd rather see
> it fixed properly. The direct caller of btrfs_full_stripe_len will be
> able to handle it.

What should be returned in case we can't find the chunk_map -EINVAL ?
> 
>> Signed-off-by: Nikolay Borisov <[e-mail]>
> 
> Reviewed-by: David Sterba <[e-mail]>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba July 12, 2017, 3:38 p.m. UTC | #3

On Tue, Jul 11, 2017 at 10:29:49PM +0300, Nikolay Borisov wrote:
> 
> 
> On 11.07.2017 20:24, David Sterba wrote:
> > On Tue, Jul 11, 2017 at 04:55:51PM +0300, Nikolay Borisov wrote:
> >> In btrfs_full_stripe_len/btrfs_is_parity_mirror we have similar code which
> >> gets the chunk map for a particular range via get_chunk_map. However,
> >> get_chunk_map can return an ERR_PTR value and while the 2 callers do catch
> >> this with a WARN_ON they then proceed to indiscriminately dereference the
> >> extent map. This of course leads to a crash. Fix the offenders by making the
> >> dereference conditional on IS_ERR.
> > 
> > While the code makes it better, the whole callchain should be fixed. The
> > WARN_ON used to be a BUG_ON and the error handling was absent, and still
> > is. Although it's unlikely to see the warnings from that, I'd rather see
> > it fixed properly. The direct caller of btrfs_full_stripe_len will be
> > able to handle it.
> 
> What should be returned in case we can't find the chunk_map -EINVAL ?

Returning what get_chunk_map seems ok in btrfs_full_stripe_len (compared
to other callers of get_chunk_map that may interpret a failure in a
different way).

But EINVAL is IMO wrong as it's more like the ENOENT when the mapping is
missing or EUCLEAN when the mapping looks incorrect.

The failure of btrfs_full_stripe_len in btrfs_create_block_group_cache
could be best handled if we return ERR_PTR instead of assuming ENOMEM in
all of its callers (btrfs_read_block_groups and btrfs_make_block_group).
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c95f018d4a1e..6959177154b0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5180,12 +5180,13 @@  unsigned long btrfs_full_stripe_len(struct btrfs_fs_info *fs_info,
 	unsigned long len = fs_info->sectorsize;
 
 	em = get_chunk_map(fs_info, logical, len);
-	WARN_ON(IS_ERR(em));
 
-	map = em->map_lookup;
-	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
-		len = map->stripe_len * nr_data_stripes(map);
-	free_extent_map(em);
+	if (!WARN_ON(IS_ERR(em))) {
+		map = em->map_lookup;
+		if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
+			len = map->stripe_len * nr_data_stripes(map);
+		free_extent_map(em);
+	}
 	return len;
 }
 
@@ -5197,12 +5198,13 @@  int btrfs_is_parity_mirror(struct btrfs_fs_info *fs_info,
 	int ret = 0;
 
 	em = get_chunk_map(fs_info, logical, len);
-	WARN_ON(IS_ERR(em));
 
-	map = em->map_lookup;
-	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
-		ret = 1;
-	free_extent_map(em);
+	if(!WARN_ON(IS_ERR(em))) {
+		map = em->map_lookup;
+		if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
+			ret = 1;
+		free_extent_map(em);
+	}
 	return ret;
 }