Btrfs: avoid NULL deref after failed allocation
PATCH 1/1 Btrfs: avoid NULL deref after failed allocation
I scanned through the sources looking mostly at kmalloc uses to see if a NULL result pointer could be dereferenced. There were a few. Where it was easy, I adjusted the code to return -ENOMEM. However, in some places, the trend is to BUG_ON(!ptr), so I've done that, too.
There were two cases where nonzero "ret" appears
to have been inadvertently ignored, so I added BUG_ON(ret) there, too, but it's possible those failures were deliberately ignored.
The second hunk doesn't matter as much: it just adds comments noting:
- a possible leak
- an invalid (or at least misleading) return code
- another possible leak
If you're interested, give a little guidance on what you want, and I'll be happy to make this more presentable.
--- fs/btrfs/file.c | 6 +++++- fs/btrfs/super.c | 6 ++++++ fs/btrfs/tree-log.c | 6 ++++++ 3 files changed, 17 insertions(+), 1 deletions(-)
fs/btrfs/file.c +5 -1
@@ -927,7 +927,11 @@ static ssize_t btrfs_file_write(struct file *file, const char __user *buf, goto out_nolock;file_update_time (file); - pages =kmalloc (nrptrs *sizeof (struct page *),GFP_KERNEL ); + pages =kmalloc (nrptrs *sizeof (*pages),GFP_KERNEL ); +if (!pages) { + err = -ENOMEM ; + goto out_nolock; + }mutex_lock (&inode->i_mutex); first_index = pos >>PAGE_CACHE_SHIFT ;
fs/btrfs/super.c +6 -0
@@ -261,11 +261,15 @@ static int btrfs_parse_early_options(const char *options, int flags, token =match_token (p, tokens, args);switch (token) {case Opt_subvol: +/* FIXME: multiple Opt_subvol options induce leak */ *subvol_name =match_strdup (&args[0 ]);break ;case Opt_device: error =btrfs_scan_one_device (match_strdup (&args[0 ]), flags, holder, fs_devices); +/* FIXME: upon match_strdup failure, fail with +-ENOMEM rather than lookup_bdev's -EINVAL. +Does the match_strdup result need to be freed? */ if (error) goto out_free_opts;break ;
@@ -531,6 +535,8 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,int len; vol =kmalloc (sizeof (*vol),GFP_KERNEL ); +if (!vol) +return -ENOMEM ;if (copy_from_user (vol, (void __user *)arg,sizeof (*vol))) { ret = -EFAULT ; goto out;
fs/btrfs/tree-log.c +6 -0
@@ -335,7 +335,9 @@ static noinline int overwrite_item(struct btrfs_trans_handle *trans,return 0 ; } dst_copy =kmalloc (item_size,GFP_NOFS ); +BUG_ON (!dst_copy); src_copy =kmalloc (item_size,GFP_NOFS ); +BUG_ON (!src_copy);read_extent_buffer (eb, src_copy, src_ptr, item_size);
@@ -462,6 +464,7 @@ insert: root->root_key.objectid, trans->transid, key->objectid, key->offset); +BUG_ON (ret); }else {/* * insert the extent pointer in the extent
@@ -633,6 +636,7 @@ static noinline int drop_one_dir_item(struct btrfs_trans_handle *trans,btrfs_dir_item_key_to_cpu (leaf, di, &location); name_len =btrfs_dir_name_len (leaf, di); name =kmalloc (name_len,GFP_NOFS ); +BUG_ON (!name);read_extent_buffer (leaf, name, (unsigned long )(di +1 ), name_len);btrfs_release_path (root, path);
@@ -2307,6 +2311,7 @@ static noinline int log_dir_items(struct btrfs_trans_handle *trans, ret =overwrite_item (trans, log, dst_path, path->nodes[0 ], path->slots[0 ], &tmp); +BUG_ON (ret); } }btrfs_release_path (root, path);
@@ -2477,6 +2482,7 @@ static noinline int copy_items(struct btrfs_trans_handle *trans, ins_data =kmalloc (nr *sizeof (struct btrfs_key ) + nr *sizeof (u32),GFP_NOFS ); +BUG_ON (!ins_data); ins_sizes = (u32 *)ins_data; ins_keys = (struct btrfs_key *)(ins_data + nr *sizeof (u32));
Signature · 1 line
1.6.0.2.27.gea240
Quoted 2 lines · Jim Meyering
However, in some places, the trend is
toBUG_ON(!ptr), so I've done that, too.
Even if it's a trend, it's wrong. Better don't add more. And also unnecessary because next reference will obviously oops anyways.
-Andi