| Message ID | 20250402034529.12642-1-hexiaole1994@126.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] fs/super.c: Add NULL check for type in iterate_supers_type | expand |
On Wed, 2025-04-02 at 11:45 +0800, Xiaole He wrote: > The first several lines of iterate_supers_type are below: > > 1 void iterate_supers_type(struct file_system_type *type, > 2 void (*f)(struct super_block *, void *), void *arg) > 3 { > 4 struct super_block *sb, *p = NULL; > 5 > 6 spin_lock(&sb_lock); > 7 hlist_for_each_entry(sb, &type->fs_supers, s_instances) { > 8 ... > 9 } > > The iterate_super_type is a exported symbol, and if > iterate_supers_type is called with type of NULL, then there will be a > NULL pointer dereference of argument type in line 7. filesystem_type is an argument to alloc_super, which the filesystems code always fills in. If a filesystem passed a NULL type to the context, the initialization code would crash on a NULL deref (iterating type->fs_supers) which makes what you're checking for here an impossible condition, doesn't it? Regards, James
On Thu, Apr 03, 2025 at 10:37:17AM +0800, Xiaole He wrote: > Thank you for your feedback. > While I acknowledge your points, I would like to clarify the rationale > behind submitting this patch. > During my experimentation with an external module interacting with the > superblock, I utilized iterate_supers_type (from fs/super.c) as it is > an exported symbol. However, I observed a potential vulnerability in > its implementation: the type argument can be passed as NULL, leading > to a null pointer dereference. To verify this, I deliberately triggered > a scenario where type was set to NULL, resulting in the following dmesg > output: > After this observasion, I worry about if this vulnerability can cause > the whole kernel crash if the type argument is passed by a > unintentional NULL in the kernel code rather than in the external > module. > Thus I submitted the patch to address the missing null-check. > Thank you for your review. You do realize that passing it NULL as the second (function pointer) argument would also oops, right? Passing (void (*)(struct super_block *))kfree there would do even more unpleasant things, etc. Sure, it's exported - so's strlen(). While we are at it, checking just for NULL is not the limit - what if the caller gives it ERR_PTR(...) as argument?
On Thu, Apr 03, 2025 at 11:10:02AM +0800, Xiaole He wrote: > Thank you for your thoughtfully feedback. > I think you are right, and I'm sorry for my pedantic anxiety. > So if we just ignore this patch for now, or I should submit a bug > report to kernel community in order to invite more thorough fix? > Thanks for your patient again. I don't believe that adding random checks would make any sense - same as for any library function, really. Having the documentation slightly more clear would make sense, though; currently it's * iterate_supers_type - call function for superblocks of given type * @type: fs type * @f: function to call * @arg: argument to pass to it * * Scans the superblock list and calls given function, passing it * locked superblock and given argument. and description could've been better. The weakest part in there is, IMO, "the superblock list" - there is a global list of all superblocks (inventively called 'super_blocks'), but that's not what gets scanned; the list of superblocks of given type (type->fs_supers) we iterate through. Something along the lines of "Call given callback @f for all superblocks of given type. The first argument passed to @f points to a locked superblock that belongs to @type; the second is a caller-supplied opaque pointer @arg. The caller is responsible for passing @arg that would make a valid second argument for @f - compiler can't help here" might be a starting point, but I'm not up to turning that into proper English.
On Wed, Apr 02, 2025 at 11:45:29AM +0800, Xiaole He wrote: > The first several lines of iterate_supers_type are below: > > 1 void iterate_supers_type(struct file_system_type *type, > 2 void (*f)(struct super_block *, void *), void *arg) > 3 { > 4 struct super_block *sb, *p = NULL; > 5 > 6 spin_lock(&sb_lock); > 7 hlist_for_each_entry(sb, &type->fs_supers, s_instances) { > 8 ... > 9 } > > The iterate_super_type is a exported symbol, and if iterate_supers_type > is called with type of NULL, then there will be a NULL pointer > dereference of argument type in line 7. > > This patch fix above problem by adding NULL pointer check for argument > type. > > Signed-off-by: Xiaole He <hexiaole1994@126.com> > --- Both Al and James already pointed out that this check is unnecessary and ultimately misleading. So we're not going to accept this patch.
diff --git a/fs/super.c b/fs/super.c index 5a7db4a556e3..105a275b8360 100644 --- a/fs/super.c +++ b/fs/super.c @@ -959,6 +959,8 @@ void iterate_supers_type(struct file_system_type *type, { struct super_block *sb, *p = NULL; + if (unlikely(ZERO_OR_NULL_PTR(type))) + return; spin_lock(&sb_lock); hlist_for_each_entry(sb, &type->fs_supers, s_instances) { bool locked;
The first several lines of iterate_supers_type are below: 1 void iterate_supers_type(struct file_system_type *type, 2 void (*f)(struct super_block *, void *), void *arg) 3 { 4 struct super_block *sb, *p = NULL; 5 6 spin_lock(&sb_lock); 7 hlist_for_each_entry(sb, &type->fs_supers, s_instances) { 8 ... 9 } The iterate_super_type is a exported symbol, and if iterate_supers_type is called with type of NULL, then there will be a NULL pointer dereference of argument type in line 7. This patch fix above problem by adding NULL pointer check for argument type. Signed-off-by: Xiaole He <hexiaole1994@126.com> --- fs/super.c | 2 ++ 1 file changed, 2 insertions(+)