Re: [PATCH] hpfs: add checks for ea addresses

[Mikulas Patocka <mpatocka@redhat.com>]

Hi

I've got an email that shows these syslog lines:

hpfs: filesystem error: warning: spare dnodes used, try chkdsk
hpfs: You really don't want any checks? You are crazy...
hpfs: hpfs_map_sector(): read error
hpfs: code page support is disabled
==================================================================
BUG: KASAN: use-after-free in strcmp+0x6f/0xc0 lib/string.c:283
Read of size 1 at addr ffff8880116728a6 by task syz-executor411/6741


It seems that you deliberately turned off checking by using the parameter 
check=none.

The HPFS driver will not check metadata for corruption if "check=none" is 
used, you should use the default "check=normal" or enable extra 
time-consuming checks using "check=strict".

The code that checks extended attributes in the fnode is in the function 
hpfs_map_fnode, the branch "if ((fnode = hpfs_map_sector(s, ino, bhp, 
FNODE_RD_AHEAD))) { if (hpfs_sb(s)->sb_chk) {" - fixes for checking 
extended attributes should go there.

If you get a KASAN warning when using "check=normal" or "check=strict", 
report it and I will fix it; with "check=none" it is not supposed to work.

Mikulas



On Sun, 20 Jul 2025, Antoni Pokusinski wrote:

> The addresses of the extended attributes are computed using the
> fnode_ea() and next_ea() functions which refer to the fields residing in
> a given fnode. There are no sanity checks for the returned values, so in
> the case of corrupted data in the fnode, the ea addresses are invalid.
> 
> Fix the bug by adding ea_valid_addr() function which checks if a given
> extended attribute resides within the range of the ea array of a given
> fnode.
> 
> Reported-by: syzbot+fa88eb476e42878f2844@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=fa88eb476e42878f2844
> Tested-by: syzbot+fa88eb476e42878f2844@syzkaller.appspotmail.com
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> 
> ---
>  fs/hpfs/anode.c   | 2 +-
>  fs/hpfs/ea.c      | 6 +++---
>  fs/hpfs/hpfs_fn.h | 5 +++++
>  fs/hpfs/map.c     | 2 +-
>  4 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/hpfs/anode.c b/fs/hpfs/anode.c
> index c14c9a035ee0..f347cdd94a5c 100644
> --- a/fs/hpfs/anode.c
> +++ b/fs/hpfs/anode.c
> @@ -488,7 +488,7 @@ void hpfs_remove_fnode(struct super_block *s, fnode_secno fno)
>  	if (!fnode_is_dir(fnode)) hpfs_remove_btree(s, &fnode->btree);
>  	else hpfs_remove_dtree(s, le32_to_cpu(fnode->u.external[0].disk_secno));
>  	ea_end = fnode_end_ea(fnode);
> -	for (ea = fnode_ea(fnode); ea < ea_end; ea = next_ea(ea))
> +	for (ea = fnode_ea(fnode); ea < ea_end && ea_valid_addr(fnode, ea); ea = next_ea(ea))
>  		if (ea_indirect(ea))
>  			hpfs_ea_remove(s, ea_sec(ea), ea_in_anode(ea), ea_len(ea));
>  	hpfs_ea_ext_remove(s, le32_to_cpu(fnode->ea_secno), fnode_in_anode(fnode), le32_to_cpu(fnode->ea_size_l));
> diff --git a/fs/hpfs/ea.c b/fs/hpfs/ea.c
> index 102ba18e561f..d7ada7f5a7ae 100644
> --- a/fs/hpfs/ea.c
> +++ b/fs/hpfs/ea.c
> @@ -80,7 +80,7 @@ int hpfs_read_ea(struct super_block *s, struct fnode *fnode, char *key,
>  	char ex[4 + 255 + 1 + 8];
>  	struct extended_attribute *ea;
>  	struct extended_attribute *ea_end = fnode_end_ea(fnode);
> -	for (ea = fnode_ea(fnode); ea < ea_end; ea = next_ea(ea))
> +	for (ea = fnode_ea(fnode); ea < ea_end  && ea_valid_addr(fnode, ea); ea = next_ea(ea))
>  		if (!strcmp(ea->name, key)) {
>  			if (ea_indirect(ea))
>  				goto indirect;
> @@ -135,7 +135,7 @@ char *hpfs_get_ea(struct super_block *s, struct fnode *fnode, char *key, int *si
>  	secno a;
>  	struct extended_attribute *ea;
>  	struct extended_attribute *ea_end = fnode_end_ea(fnode);
> -	for (ea = fnode_ea(fnode); ea < ea_end; ea = next_ea(ea))
> +	for (ea = fnode_ea(fnode); ea < ea_end && ea_valid_addr(fnode, ea); ea = next_ea(ea))
>  		if (!strcmp(ea->name, key)) {
>  			if (ea_indirect(ea))
>  				return get_indirect_ea(s, ea_in_anode(ea), ea_sec(ea), *size = ea_len(ea));
> @@ -198,7 +198,7 @@ void hpfs_set_ea(struct inode *inode, struct fnode *fnode, const char *key,
>  	unsigned char h[4];
>  	struct extended_attribute *ea;
>  	struct extended_attribute *ea_end = fnode_end_ea(fnode);
> -	for (ea = fnode_ea(fnode); ea < ea_end; ea = next_ea(ea))
> +	for (ea = fnode_ea(fnode); ea < ea_end && ea_valid_addr(fnode, ea); ea = next_ea(ea))
>  		if (!strcmp(ea->name, key)) {
>  			if (ea_indirect(ea)) {
>  				if (ea_len(ea) == size)
> diff --git a/fs/hpfs/hpfs_fn.h b/fs/hpfs/hpfs_fn.h
> index 237c1c23e855..c65ce60d7d9a 100644
> --- a/fs/hpfs/hpfs_fn.h
> +++ b/fs/hpfs/hpfs_fn.h
> @@ -152,6 +152,11 @@ static inline struct extended_attribute *next_ea(struct extended_attribute *ea)
>  	return (struct extended_attribute *)((char *)ea + 5 + ea->namelen + ea_valuelen(ea));
>  }
>  
> +static inline bool ea_valid_addr(struct fnode *fnode, struct extended_attribute *ea)
> +{
> +	return ((char *)ea >= (char *)&fnode->ea) && ((char *)ea < (char *)&fnode->ea + sizeof(fnode->ea));
> +}
> +
>  static inline secno ea_sec(struct extended_attribute *ea)
>  {
>  	return le32_to_cpu(get_unaligned((__le32 *)((char *)ea + 9 + ea->namelen)));
> diff --git a/fs/hpfs/map.c b/fs/hpfs/map.c
> index ecd9fccd1663..0016dcbf1b1f 100644
> --- a/fs/hpfs/map.c
> +++ b/fs/hpfs/map.c
> @@ -202,7 +202,7 @@ struct fnode *hpfs_map_fnode(struct super_block *s, ino_t ino, struct buffer_hea
>  			}
>  			ea = fnode_ea(fnode);
>  			ea_end = fnode_end_ea(fnode);
> -			while (ea != ea_end) {
> +			while (ea != ea_end && ea_valid_addr(fnode, ea)) {
>  				if (ea > ea_end) {
>  					hpfs_error(s, "bad EA in fnode %08lx",
>  						(unsigned long)ino);
> -- 
> 2.25.1
>