diff mbox series

btrfs: sysfs: Handle NULL return values

Message ID 20230204183510.never.909-kees@kernel.org (mailing list archive)
State Superseded
Headers show
Series btrfs: sysfs: Handle NULL return values | expand

Commit Message

Kees Cook Feb. 4, 2023, 6:35 p.m. UTC 
Each of to_fs_info(), discard_to_fs_info(), and to_space_info() can
return NULL values. Check for these so it's not possible to perform
calculations against NULL pointers. Fixes many warnings seen with GCC 13
like:

In function 'btrfs_show_u64',
    inlined from 'btrfs_space_info_show_flags' at ../fs/btrfs/sysfs.c:827:1:
../fs/btrfs/sysfs.c:636:13: warning: array subscript -50 is outside array bounds of 'struct kobject[36028797018963967]' [-Warray-bounds=]
  636 |         val = *value_ptr;
      |         ~~~~^~~~~~~~~~~~

Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/btrfs/sysfs.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

Comments

Greg KH Feb. 5, 2023, 7:13 a.m. UTC | #1 
On Sat, Feb 04, 2023 at 10:35:10AM -0800, Kees Cook wrote:
> Each of to_fs_info(), discard_to_fs_info(), and to_space_info() can
> return NULL values.

The code says it could, but I really do not think that is possible at
all, especially based on the fact that there have never been any crashes
reported here.

So the NULL returns should just be removed instead, right?

Also, to_space_info() is a macro of container_of() which can not return
NULL.

And get_btrfs_kobj() is just odd, that probably should be fixed up as
well, that's an indication that something is wrong with the sysfs code
if anyone has to attempt to walk the whole kobject parent path :(

So while this patch might fix up the compiler warning, it's logically
not going to change anything, let's fix this properly.  I can look at it
next week if someone doesn't beat me to it.

thanks,

greg k-h
David Sterba Feb. 6, 2023, 6:33 p.m. UTC | #2 
On Sun, Feb 05, 2023 at 08:13:30AM +0100, Greg KH wrote:
> On Sat, Feb 04, 2023 at 10:35:10AM -0800, Kees Cook wrote:
> > Each of to_fs_info(), discard_to_fs_info(), and to_space_info() can
> > return NULL values.
> 
> The code says it could, but I really do not think that is possible at
> all, especially based on the fact that there have never been any crashes
> reported here.
> 
> So the NULL returns should just be removed instead, right?

Yes. The access to any sysfs callback always implies an existing
fs_info, the sysfs files are destroyed before the instance of fs_info.
If the callback is slow or stuck for some reason, the removal will block
due to reference counts.

> Also, to_space_info() is a macro of container_of() which can not return
> NULL.
> 
> And get_btrfs_kobj() is just odd, that probably should be fixed up as
> well, that's an indication that something is wrong with the sysfs code
> if anyone has to attempt to walk the whole kobject parent path :(

The get_btrfs_kobj is relatively new and more generic than the other
helpers that basically hardcoded the kobj path relative to the main
sysfs object.

> So while this patch might fix up the compiler warning, it's logically
> not going to change anything, let's fix this properly.  I can look at it
> next week if someone doesn't beat me to it.

Let me have a look, I was meaning to clean up the code to use only
get_btrfs_kobj. So far there was no pressing reason for that but if gcc
13 is going to warn about the checks it needs to be fixed.
Kees Cook Feb. 6, 2023, 6:37 p.m. UTC | #3 
On Sun, Feb 05, 2023 at 08:13:30AM +0100, Greg KH wrote:
> On Sat, Feb 04, 2023 at 10:35:10AM -0800, Kees Cook wrote:
> > Each of to_fs_info(), discard_to_fs_info(), and to_space_info() can
> > return NULL values.
> 
> The code says it could, but I really do not think that is possible at
> all, especially based on the fact that there have never been any crashes
> reported here.

I'm not sure that's a useful measure if we're trying to improve
robustness under memory corruption, but at least one of those helpers is
performing a type check, not just a simple container_of(), etc.

Regardless, yeah, if this can be done without NULL returns, sure, let's
do it. I just don't know this code well enough to say what's possible.
:)
diff mbox series

Patch

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 108aa3876186..01e0f439d8e6 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -444,6 +444,8 @@  static ssize_t btrfs_discardable_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%lld\n",
 			atomic64_read(&fs_info->discard_ctl.discardable_bytes));
 }
@@ -455,6 +457,8 @@  static ssize_t btrfs_discardable_extents_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%d\n",
 			atomic_read(&fs_info->discard_ctl.discardable_extents));
 }
@@ -466,6 +470,8 @@  static ssize_t btrfs_discard_bitmap_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%llu\n",
 			  fs_info->discard_ctl.discard_bitmap_bytes);
 }
@@ -477,6 +483,8 @@  static ssize_t btrfs_discard_bytes_saved_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%lld\n",
 		atomic64_read(&fs_info->discard_ctl.discard_bytes_saved));
 }
@@ -488,6 +496,8 @@  static ssize_t btrfs_discard_extent_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%llu\n",
 			  fs_info->discard_ctl.discard_extent_bytes);
 }
@@ -499,6 +509,8 @@  static ssize_t btrfs_discard_iops_limit_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%u\n",
 			  READ_ONCE(fs_info->discard_ctl.iops_limit));
 }
@@ -512,6 +524,8 @@  static ssize_t btrfs_discard_iops_limit_store(struct kobject *kobj,
 	u32 iops_limit;
 	int ret;
 
+	if (!fs_info)
+		return -EINVAL;
 	ret = kstrtou32(buf, 10, &iops_limit);
 	if (ret)
 		return -EINVAL;
@@ -530,6 +544,8 @@  static ssize_t btrfs_discard_kbps_limit_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%u\n",
 			  READ_ONCE(fs_info->discard_ctl.kbps_limit));
 }
@@ -543,6 +559,9 @@  static ssize_t btrfs_discard_kbps_limit_store(struct kobject *kobj,
 	u32 kbps_limit;
 	int ret;
 
+	if (!fs_info)
+		return -EINVAL;
+
 	ret = kstrtou32(buf, 10, &kbps_limit);
 	if (ret)
 		return -EINVAL;
@@ -560,6 +579,8 @@  static ssize_t btrfs_discard_max_discard_size_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%llu\n",
 			  READ_ONCE(fs_info->discard_ctl.max_discard_size));
 }
@@ -573,6 +594,8 @@  static ssize_t btrfs_discard_max_discard_size_store(struct kobject *kobj,
 	u64 max_discard_size;
 	int ret;
 
+	if (!fs_info)
+		return -EINVAL;
 	ret = kstrtou64(buf, 10, &max_discard_size);
 	if (ret)
 		return -EINVAL;
@@ -644,6 +667,9 @@  static ssize_t global_rsv_size_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj->parent);
 	struct btrfs_block_rsv *block_rsv = &fs_info->global_block_rsv;
+
+	if (!fs_info)
+		return -EINVAL;
 	return btrfs_show_u64(&block_rsv->size, &block_rsv->lock, buf);
 }
 BTRFS_ATTR(allocation, global_rsv_size, global_rsv_size_show);
@@ -653,6 +679,9 @@  static ssize_t global_rsv_reserved_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj->parent);
 	struct btrfs_block_rsv *block_rsv = &fs_info->global_block_rsv;
+
+	if (!fs_info)
+		return -EINVAL;
 	return btrfs_show_u64(&block_rsv->reserved, &block_rsv->lock, buf);
 }
 BTRFS_ATTR(allocation, global_rsv_reserved, global_rsv_reserved_show);
@@ -714,6 +743,9 @@  static ssize_t btrfs_space_info_show_##field(struct kobject *kobj,	\
 					     char *buf)			\
 {									\
 	struct btrfs_space_info *sinfo = to_space_info(kobj);		\
+									\
+	if (!sinfo)							\
+		return -EINVAL;						\
 	return btrfs_show_u64(&sinfo->field, &sinfo->lock, buf);	\
 }									\
 BTRFS_ATTR(space_info, field, btrfs_space_info_show_##field)
@@ -723,6 +755,8 @@  static ssize_t btrfs_chunk_size_show(struct kobject *kobj,
 {
 	struct btrfs_space_info *sinfo = to_space_info(kobj);
 
+	if (!sinfo)
+		return -EINVAL;
 	return sysfs_emit(buf, "%llu\n", READ_ONCE(sinfo->chunk_size));
 }
 
@@ -745,6 +779,9 @@  static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (!space_info || !fs_info)
+		return -EINVAL;
+
 	if (!fs_info->fs_devices)
 		return -EINVAL;
 
@@ -795,6 +832,9 @@  static ssize_t btrfs_force_chunk_alloc_store(struct kobject *kobj,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (!space_info || !fs_info)
+		return -EINVAL;
+
 	if (sb_rdonly(fs_info->sb))
 		return -EROFS;
 
@@ -842,6 +882,8 @@  static ssize_t btrfs_sinfo_bg_reclaim_threshold_show(struct kobject *kobj,
 {
 	struct btrfs_space_info *space_info = to_space_info(kobj);
 
+	if (!space_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%d\n", READ_ONCE(space_info->bg_reclaim_threshold));
 }
 
@@ -853,6 +895,9 @@  static ssize_t btrfs_sinfo_bg_reclaim_threshold_store(struct kobject *kobj,
 	int thresh;
 	int ret;
 
+	if (!space_info)
+		return -EINVAL;
+
 	ret = kstrtoint(buf, 10, &thresh);
 	if (ret)
 		return ret;
@@ -924,6 +969,9 @@  static ssize_t btrfs_label_show(struct kobject *kobj,
 	char *label = fs_info->super_copy->label;
 	ssize_t ret;
 
+	if (!fs_info)
+		return -EINVAL;
+
 	spin_lock(&fs_info->super_lock);
 	ret = sysfs_emit(buf, label[0] ? "%s\n" : "%s", label);
 	spin_unlock(&fs_info->super_lock);
@@ -973,6 +1021,8 @@  static ssize_t btrfs_nodesize_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%u\n", fs_info->super_copy->nodesize);
 }
 
@@ -983,6 +1033,8 @@  static ssize_t btrfs_sectorsize_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%u\n", fs_info->super_copy->sectorsize);
 }
 
@@ -993,6 +1045,8 @@  static ssize_t btrfs_commit_stats_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf,
 		"commits %llu\n"
 		"last_commit_ms %llu\n"
@@ -1035,6 +1089,8 @@  static ssize_t btrfs_clone_alignment_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%u\n", fs_info->super_copy->sectorsize);
 }
 
@@ -1046,6 +1102,8 @@  static ssize_t quota_override_show(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	int quota_override;
 
+	if (!fs_info)
+		return -EINVAL;
 	quota_override = test_bit(BTRFS_FS_QUOTA_OVERRIDE, &fs_info->flags);
 	return sysfs_emit(buf, "%d\n", quota_override);
 }
@@ -1085,6 +1143,8 @@  static ssize_t btrfs_metadata_uuid_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%pU\n", fs_info->fs_devices->metadata_uuid);
 }
 
@@ -1094,8 +1154,11 @@  static ssize_t btrfs_checksum_show(struct kobject *kobj,
 				   struct kobj_attribute *a, char *buf)
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
-	u16 csum_type = btrfs_super_csum_type(fs_info->super_copy);
+	u16 csum_type;
 
+	if (!fs_info)
+		return -EINVAL;
+	csum_type = btrfs_super_csum_type(fs_info->super_copy);
 	return sysfs_emit(buf, "%s (%s)\n",
 			  btrfs_super_csum_name(csum_type),
 			  crypto_shash_driver_name(fs_info->csum_shash));
@@ -1109,6 +1172,8 @@  static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	const char *str;
 
+	if (!fs_info)
+		return -EINVAL;
 	switch (READ_ONCE(fs_info->exclusive_operation)) {
 		case  BTRFS_EXCLOP_NONE:
 			str = "none\n";
@@ -1147,6 +1212,8 @@  static ssize_t btrfs_generation_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%llu\n", fs_info->generation);
 }
 BTRFS_ATTR(, generation, btrfs_generation_show);
@@ -1205,6 +1272,8 @@  static ssize_t btrfs_bg_reclaim_threshold_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
+	if (!fs_info)
+		return -EINVAL;
 	return sysfs_emit(buf, "%d\n", READ_ONCE(fs_info->bg_reclaim_threshold));
 }
 
@@ -1216,6 +1285,9 @@  static ssize_t btrfs_bg_reclaim_threshold_store(struct kobject *kobj,
 	int thresh;
 	int ret;
 
+	if (!fs_info)
+		return -EINVAL;
+
 	ret = kstrtoint(buf, 10, &thresh);
 	if (ret)
 		return ret;
@@ -1273,6 +1345,8 @@  static inline struct btrfs_fs_devices *to_fs_devs(struct kobject *kobj)
 
 static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj)
 {
+	if (!kobj)
+		return NULL;
 	if (kobj->ktype != &btrfs_ktype)
 		return NULL;
 	return to_fs_devs(kobj)->fs_info;