[PATCH] hpfs: add checks for ea addresses

[PATCH] hpfs: add checks for ea addresses

[Antoni Pokusinski]

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

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

[Mikulas Patocka]

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
> 

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

[Antoni Pokusinski]

Hello,
Thanks for the feedback.

On Mon, Jul 21, 2025 at 09:51:22PM +0200, Mikulas Patocka wrote:
> 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".
> 

Yes, that's right. If we had "check!=none", then the issue would not come
up in syzkaller due to the checks performed on the extended attribues in the fnode.

I've just tried to confim that by using the "check=normal" and I did not get
the KASAN warning, as expected.

> 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
> 

I'm just wondering what should be the expected kernel behaviour in the situation where
"check=none" and the "ea_offs", "acl_size_s", "ea_size_s" fields of fnode are corrupt?
If we assume that in such case running into some undefined behavior (which is accessing
an unknown memory area) is alright, then the code does not need any changes.
But if we'd like to prevent it, then I think we should always check the extended
attribute address regardless of the "check" parameter, as demonstrated
in the patch.

Kind regards,
Antoni

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

[Mikulas Patocka]

On Tue, 22 Jul 2025, Antoni Pokusinski wrote:

> > 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
> > 
> 
> I'm just wondering what should be the expected kernel behaviour in the situation where
> "check=none" and the "ea_offs", "acl_size_s", "ea_size_s" fields of fnode are corrupt?
> If we assume that in such case running into some undefined behavior (which is accessing
> an unknown memory area) is alright, then the code does not need any changes.
> But if we'd like to prevent it, then I think we should always check the extended
> attribute address regardless of the "check" parameter, as demonstrated
> in the patch.
> 
> Kind regards,
> Antoni

There is a trade-off between speed and resiliency. If the user wants 
maximum speed and uses the filesystem only on trusted input, he can choose 
"check=none". If the user wants less performance and uses the filesystem 
on untrusted input, he can select "check=normal" (the default). If the 
user is modifying the code and wants maximum safeguards, he should select 
"check=strict" (that will degrade performance significantly, but it will 
stop the filesystem as soon as possible if something goes wrong).

I think there is no need to add some middle ground where "check=none" 
would check some structures and won't check others.

Mikulas

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

[Antoni Pokusinski]

On Thu, Jul 24, 2025 at 04:21:47PM +0200, Mikulas Patocka wrote:
> 
> 
> On Tue, 22 Jul 2025, Antoni Pokusinski wrote:
> 
> > > 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
> > > 
> > 
> > I'm just wondering what should be the expected kernel behaviour in the situation where
> > "check=none" and the "ea_offs", "acl_size_s", "ea_size_s" fields of fnode are corrupt?
> > If we assume that in such case running into some undefined behavior (which is accessing
> > an unknown memory area) is alright, then the code does not need any changes.
> > But if we'd like to prevent it, then I think we should always check the extended
> > attribute address regardless of the "check" parameter, as demonstrated
> > in the patch.
> > 
> > Kind regards,
> > Antoni
> 
> There is a trade-off between speed and resiliency. If the user wants 
> maximum speed and uses the filesystem only on trusted input, he can choose 
> "check=none". If the user wants less performance and uses the filesystem 
> on untrusted input, he can select "check=normal" (the default). If the 
> user is modifying the code and wants maximum safeguards, he should select 
> "check=strict" (that will degrade performance significantly, but it will 
> stop the filesystem as soon as possible if something goes wrong).
> 
> I think there is no need to add some middle ground where "check=none" 
> would check some structures and won't check others.
> 
> Mikulas
>

Thanks for the explanation. Yeah I think I agree with your point, I
guess that the patch is not necessary then.

Kind regards,
Antoni