Re: [PATCH 2/4] ext4: Expose the filesystem encoding through sysfs

[Gabriel Krisman Bertazi <krisman@suse.de>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ext4: NULL pointer deref in encoding_show() via dynamic casefold
> --
>
> commit c325498f77eea14582e06218baa713f49fde001a
> Author: Gabriel Krisman Bertazi <krisman@suse.de>
>
> ext4: Expose the filesystem encoding through sysfs
>
> This commit introduces new sysfs attributes in ext4 to display the active
> filesystem encoding and encoding flags. This provides users with a direct
> interface to check casefold support without relying on dmesg parsing.
>
>> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
>> index 923b375e017fa..bc1c59d67eed8 100644
>> --- a/fs/ext4/sysfs.c
>> +++ b/fs/ext4/sysfs.c
> [ ... ]
>> @@ -161,6 +163,25 @@ static ssize_t err_report_sec_store(struct ext4_sb_info *sbi,
>>  	return count;
>>  }
>>  
>> +static ssize_t encoding_show(struct ext4_sb_info *sbi, char *buf)
>> +{
>> +#if IS_ENABLED(CONFIG_UNICODE)
>> +	struct super_block *sb = sbi->s_sb;
>> +
>> +	if (ext4_has_feature_casefold(sb))
>> +		return sysfs_emit(buf, "UTF-8 (%d.%d.%d)\n",
>> +				  unicode_major(sb->s_encoding->version),
>> +				  unicode_minor(sb->s_encoding->version),
>> +				  unicode_rev(sb->s_encoding->version));
>
> [Severity: High]
> Does this introduce a regression where reading the sysfs attribute causes a
> NULL pointer dereference if casefold is enabled dynamically?
>
> If a privileged user dynamically enables EXT4_FEATURE_INCOMPAT_CASEFOLD on
> an online filesystem via the EXT4_IOC_SET_TUNE_SB_PARAM ioctl, it updates
> the in-memory on-disk superblock representation but does not load the
> unicode map into the VFS superblock:
>
> fs/ext4/ioctl.c:ext4_ioctl_set_tune_sb() {
>     ...
>     if ((params.set_feature_incompat_mask &
>          EXT4_FEATURE_INCOMPAT_CASEFOLD)) {
>         enabling_casefold = 1;
>     ...
> }
>
> In this state, ext4_has_feature_casefold(sb) evaluates to true, but
> sb->s_encoding remains NULL. If the sysfs attribute is read,
> encoding_show() will unconditionally dereference sb->s_encoding->version,
> resulting in a system crash.

Uh...

If we flip EXT4_FEATURE_INCOMPAT_CASEFOLD without loading the casefold
table, I'm surprised the current code doesn't blow up already.  I
completely missed the introduction of EXT4_IOC_SET_TUNE_SB_PARAM, so I'm
missing context if there is a use case to actually do an online flip of
feat_casefold.  It seems the ioctl actually can set this bit, but I
don't see any provision to load the table.  I'm a bit confused now.  I
can work around it for /sys/fs, but is anyone actually enabling casefold
without a remount?

>
>> +#endif
>> +	return sysfs_emit(buf, "(none)\n");
>> +}
>> +
> [ ... ]
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260928213155.2612039-1-krisman@suse.de?part=2

-- 
Gabriel Krisman Bertazi