[PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Namjae Jeon]

The compatibility issue between linux exfat and exfat of some camera
company was reported from Florian. In their exfat, if the number of files
exceeds any limit, the DataLength in stream entry of the directory is
no longer updated. So some files created from camera does not show in
linux exfat. because linux exfat doesn't allow that cpos becomes larger
than DataLength of stream entry. This patch check DataLength in stream
entry only if the type is ALLOC_NO_FAT_CHAIN and add the check ensure
that dentry offset does not exceed max dentries size(256 MB) to avoid
the circular FAT chain issue.

Fixes: ca06197382bd ("exfat: add directory operations")
Cc: stable@vger.kernel.org # v5.9
Reported-by: Florian Cramer <flrncrmr@gmail.com>
Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
---
 fs/exfat/dir.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index c4523648472a..f4e4d8d9894d 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -63,7 +63,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
 static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_entry *dir_entry)
 {
 	int i, dentries_per_clu, dentries_per_clu_bits = 0, num_ext;
-	unsigned int type, clu_offset;
+	unsigned int type, clu_offset, max_dentries;
 	sector_t sector;
 	struct exfat_chain dir, clu;
 	struct exfat_uni_name uni_name;
@@ -86,6 +86,8 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
 
 	dentries_per_clu = sbi->dentries_per_clu;
 	dentries_per_clu_bits = ilog2(dentries_per_clu);
+	max_dentries = (unsigned int)min_t(u64, MAX_EXFAT_DENTRIES,
+					   (u64)sbi->num_clusters << dentries_per_clu_bits);
 
 	clu_offset = dentry >> dentries_per_clu_bits;
 	exfat_chain_dup(&clu, &dir);
@@ -109,7 +111,7 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
 		}
 	}
 
-	while (clu.dir != EXFAT_EOF_CLUSTER) {
+	while (clu.dir != EXFAT_EOF_CLUSTER && dentry < max_dentries) {
 		i = dentry & (dentries_per_clu - 1);
 
 		for ( ; i < dentries_per_clu; i++, dentry++) {
@@ -245,7 +247,7 @@ static int exfat_iterate(struct file *filp, struct dir_context *ctx)
 	if (err)
 		goto unlock;
 get_new:
-	if (cpos >= i_size_read(inode))
+	if (ei->flags == ALLOC_NO_FAT_CHAIN && cpos >= i_size_read(inode))
 		goto end_of_dir;
 
 	err = exfat_readdir(inode, &cpos, &de);
-- 
2.17.1

Re: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Chris Down]

Namjae Jeon writes:
>The compatibility issue between linux exfat and exfat of some camera
>company was reported from Florian. In their exfat, if the number of files
>exceeds any limit, the DataLength in stream entry of the directory is
>no longer updated. So some files created from camera does not show in
>linux exfat. because linux exfat doesn't allow that cpos becomes larger
>than DataLength of stream entry. This patch check DataLength in stream
>entry only if the type is ALLOC_NO_FAT_CHAIN and add the check ensure
>that dentry offset does not exceed max dentries size(256 MB) to avoid
>the circular FAT chain issue.
>
>Fixes: ca06197382bd ("exfat: add directory operations")
>Cc: stable@vger.kernel.org # v5.9
>Reported-by: Florian Cramer <flrncrmr@gmail.com>
>Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
>Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>

Tested-by: Chris Down <chris@chrisdown.name>

Thanks, I came across this while debugging why directories produced on my Fuji 
X-T4 were truncated at 2^12 dentries.

If the other report was also Fuji, maybe this is worth asking them to fix in 
firmware?

>---
> fs/exfat/dir.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
>index c4523648472a..f4e4d8d9894d 100644
>--- a/fs/exfat/dir.c
>+++ b/fs/exfat/dir.c
>@@ -63,7 +63,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_entry *dir_entry)
> {
> 	int i, dentries_per_clu, dentries_per_clu_bits = 0, num_ext;
>-	unsigned int type, clu_offset;
>+	unsigned int type, clu_offset, max_dentries;
> 	sector_t sector;
> 	struct exfat_chain dir, clu;
> 	struct exfat_uni_name uni_name;
>@@ -86,6 +86,8 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
>
> 	dentries_per_clu = sbi->dentries_per_clu;
> 	dentries_per_clu_bits = ilog2(dentries_per_clu);
>+	max_dentries = (unsigned int)min_t(u64, MAX_EXFAT_DENTRIES,
>+					   (u64)sbi->num_clusters << dentries_per_clu_bits);
>
> 	clu_offset = dentry >> dentries_per_clu_bits;
> 	exfat_chain_dup(&clu, &dir);
>@@ -109,7 +111,7 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
> 		}
> 	}
>
>-	while (clu.dir != EXFAT_EOF_CLUSTER) {
>+	while (clu.dir != EXFAT_EOF_CLUSTER && dentry < max_dentries) {
> 		i = dentry & (dentries_per_clu - 1);
>
> 		for ( ; i < dentries_per_clu; i++, dentry++) {
>@@ -245,7 +247,7 @@ static int exfat_iterate(struct file *filp, struct dir_context *ctx)
> 	if (err)
> 		goto unlock;
> get_new:
>-	if (cpos >= i_size_read(inode))
>+	if (ei->flags == ALLOC_NO_FAT_CHAIN && cpos >= i_size_read(inode))
> 		goto end_of_dir;
>
> 	err = exfat_readdir(inode, &cpos, &de);
>-- 
>2.17.1
>

RE: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Namjae Jeon]

> Namjae Jeon writes:
> >The compatibility issue between linux exfat and exfat of some camera
> >company was reported from Florian. In their exfat, if the number of
> >files exceeds any limit, the DataLength in stream entry of the
> >directory is no longer updated. So some files created from camera does
> >not show in linux exfat. because linux exfat doesn't allow that cpos
> >becomes larger than DataLength of stream entry. This patch check
> >DataLength in stream entry only if the type is ALLOC_NO_FAT_CHAIN and
> >add the check ensure that dentry offset does not exceed max dentries
> >size(256 MB) to avoid the circular FAT chain issue.
> >
> >Fixes: ca06197382bd ("exfat: add directory operations")
> >Cc: stable@vger.kernel.org # v5.9
> >Reported-by: Florian Cramer <flrncrmr@gmail.com>
> >Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> >Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> 
> Tested-by: Chris Down <chris@chrisdown.name>
Thanks for your test!
> 
> Thanks, I came across this while debugging why directories produced on my Fuji
> X-T4 were truncated at 2^12 dentries.
> 
> If the other report was also Fuji, maybe this is worth asking them to fix in firmware?
Well, I am not sure that they will respond to your report well. If you can
reproduce same issue even when plugging your exfat into windows, I think
that it is worth reporting to them.

> >---
> > fs/exfat/dir.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> >diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> >c4523648472a..f4e4d8d9894d 100644
> >--- a/fs/exfat/dir.c
> >+++ b/fs/exfat/dir.c
> >@@ -63,7 +63,7 @@ static void exfat_get_uniname_from_ext_entry(struct
> >super_block *sb,  static int exfat_readdir(struct inode *inode, loff_t
> >*cpos, struct exfat_dir_entry *dir_entry)  {
> > 	int i, dentries_per_clu, dentries_per_clu_bits = 0, num_ext;
> >-	unsigned int type, clu_offset;
> >+	unsigned int type, clu_offset, max_dentries;
> > 	sector_t sector;
> > 	struct exfat_chain dir, clu;
> > 	struct exfat_uni_name uni_name;
> >@@ -86,6 +86,8 @@ static int exfat_readdir(struct inode *inode, loff_t
> >*cpos, struct exfat_dir_ent
> >
> > 	dentries_per_clu = sbi->dentries_per_clu;
> > 	dentries_per_clu_bits = ilog2(dentries_per_clu);
> >+	max_dentries = (unsigned int)min_t(u64, MAX_EXFAT_DENTRIES,
> >+					   (u64)sbi->num_clusters << dentries_per_clu_bits);
> >
> > 	clu_offset = dentry >> dentries_per_clu_bits;
> > 	exfat_chain_dup(&clu, &dir);
> >@@ -109,7 +111,7 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
> > 		}
> > 	}
> >
> >-	while (clu.dir != EXFAT_EOF_CLUSTER) {
> >+	while (clu.dir != EXFAT_EOF_CLUSTER && dentry < max_dentries) {
> > 		i = dentry & (dentries_per_clu - 1);
> >
> > 		for ( ; i < dentries_per_clu; i++, dentry++) { @@ -245,7 +247,7 @@
> >static int exfat_iterate(struct file *filp, struct dir_context *ctx)
> > 	if (err)
> > 		goto unlock;
> > get_new:
> >-	if (cpos >= i_size_read(inode))
> >+	if (ei->flags == ALLOC_NO_FAT_CHAIN && cpos >= i_size_read(inode))
> > 		goto end_of_dir;
> >
> > 	err = exfat_readdir(inode, &cpos, &de);
> >--
> >2.17.1
> >

RE: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Kohada.Tetsuhiro]

> The compatibility issue between linux exfat and exfat of some camera company was reported from Florian. In their exfat,
> if the number of files exceeds any limit, the DataLength in stream entry of the directory is no longer updated. So some files
> created from camera does not show in linux exfat. because linux exfat doesn't allow that cpos becomes larger than
> DataLength of stream entry. This patch check DataLength in stream entry only if the type is ALLOC_NO_FAT_CHAIN and
> add the check ensure that dentry offset does not exceed max dentries size(256 MB) to avoid the circular FAT chain issue.

Instead of using fsd to handle this, shouldn't it be left to fsck?

In the exfat specification says, the DataLength Field of the directory-stream is the entire size of the associated allocation.
If the DataLength Field does not match the size in the FAT-chain, it means that it is corrupted.

As you know, the FAT-chain structure is fragile.
At runtime, one way to detect a broken FAT-chain is to compare it with DataLength.
(Detailed verification is the role of fsck).
Ignoring DataLength during dir-scan is unsafe because we lose a way to detect a broken FAT-chain.

I think fsd should check DataLength, and fsck should repair DataLength.

As for the 256MB check, I think it would be better to have it.

BR
---
Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

RE: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Namjae Jeon]

> > The compatibility issue between linux exfat and exfat of some camera
> > company was reported from Florian. In their exfat, if the number of
> > files exceeds any limit, the DataLength in stream entry of the
> > directory is no longer updated. So some files created from camera does
> > not show in linux exfat. because linux exfat doesn't allow that cpos becomes larger than DataLength
> of stream entry. This patch check DataLength in stream entry only if the type is ALLOC_NO_FAT_CHAIN
> and add the check ensure that dentry offset does not exceed max dentries size(256 MB) to avoid the
> circular FAT chain issue.
> 
> Instead of using fsd to handle this, shouldn't it be left to fsck?
Yes, That's what I thought at first. And fsck.exfat in exfatprogs can detect it like this.

$ fsck.exfat /dev/sdb1
exfatprogs version : 1.1.1
ERROR: /DCIM/344_FUJI: more clusters are allocated. truncate to 524288 bytes. Truncate (y/N)? n

> 
> In the exfat specification says, the DataLength Field of the directory-stream is the entire size of
> the associated allocation.
> If the DataLength Field does not match the size in the FAT-chain, it means that it is corrupted.
Yes. I have checked it.
> 
> As you know, the FAT-chain structure is fragile.
> At runtime, one way to detect a broken FAT-chain is to compare it with DataLength.
> (Detailed verification is the role of fsck).
> Ignoring DataLength during dir-scan is unsafe because we lose a way to detect a broken FAT-chain.
> 
> I think fsd should check DataLength, and fsck should repair DataLength.
But Windows fsck doesn’t detect it and it shows the all files normally without any missing ones.
It means Windows exfat doesn't also check it in case type is ALLOC_FAT_CHAIN.

> 
> As for the 256MB check, I think it would be better to have it.
> 
> BR
> ---
> Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

RE: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Kohada.Tetsuhiro]

> > > The compatibility issue between linux exfat and exfat of some camera
> > > company was reported from Florian. In their exfat, if the number of
> > > files exceeds any limit, the DataLength in stream entry of the
> > > directory is no longer updated. So some files created from camera
> > > does not show in linux exfat. because linux exfat doesn't allow that
> > > cpos becomes larger than DataLength
> > of stream entry. This patch check DataLength in stream entry only if
> > the type is ALLOC_NO_FAT_CHAIN and add the check ensure that dentry
> > offset does not exceed max dentries size(256 MB) to avoid the circular FAT chain issue.
> >
> > Instead of using fsd to handle this, shouldn't it be left to fsck?
> Yes, That's what I thought at first. And fsck.exfat in exfatprogs can detect it like this.
> 
> $ fsck.exfat /dev/sdb1
> exfatprogs version : 1.1.1
> ERROR: /DCIM/344_FUJI: more clusters are allocated. truncate to 524288 bytes. Truncate (y/N)? n
> 
> >
> > In the exfat specification says, the DataLength Field of the
> > directory-stream is the entire size of the associated allocation.
> > If the DataLength Field does not match the size in the FAT-chain, it means that it is corrupted.
> Yes. I have checked it.
> >
> > As you know, the FAT-chain structure is fragile.
> > At runtime, one way to detect a broken FAT-chain is to compare it with DataLength.
> > (Detailed verification is the role of fsck).
> > Ignoring DataLength during dir-scan is unsafe because we lose a way to detect a broken FAT-chain.
> >
> > I think fsd should check DataLength, and fsck should repair DataLength.
> But Windows fsck doesn’t detect it and it shows the all files normally without any missing ones.
> It means Windows exfat doesn't also check it in case type is ALLOC_FAT_CHAIN.

Okay, I get it.
Because if our driver aborts scanning, the files after that will not be visible.
If FAT-Chain is correct and DataLenth is incorrect,  this patch works safely.(as the case of Fuji)
However, if DataLenth is correct and FAT-Chain is incorrect, it is unsafe.
There is a risk of destroying other files/dirs.

The current implementation sets SB_RDONLY when it detects a directories FAT-Chain count and DataLenth contradiction in exfat_find_last_cluster( ).
It only works only on exfat_add_entry()/exfat_rename_file()/exfat_move_file().

Why don't we do a similar check for exfat_readdir()/exfat_find_dir_entry()?
Just do exfat_fs_error() if the dentry position exceeds DataLenth in the scan loop.
This will reduces the risk of destroying other files/dirs and prompts the user for fsck.


> Fixes: ca06197382bd ("exfat: add directory operations")
> Cc: stable@vger.kernel.org # v5.9
> Reported-by: Florian Cramer <flrncrmr@gmail.com>
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> ---
>  fs/exfat/dir.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index c4523648472a..f4e4d8d9894d 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -63,7 +63,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,  static int
> exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_entry *dir_entry)  {
>  	int i, dentries_per_clu, dentries_per_clu_bits = 0, num_ext;
> -	unsigned int type, clu_offset;
> +	unsigned int type, clu_offset, max_dentries;
>  	sector_t sector;
>  	struct exfat_chain dir, clu;
>  	struct exfat_uni_name uni_name;
> @@ -86,6 +86,8 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
> 
>  	dentries_per_clu = sbi->dentries_per_clu;
>  	dentries_per_clu_bits = ilog2(dentries_per_clu);
> +	max_dentries = (unsigned int)min_t(u64, MAX_EXFAT_DENTRIES,
> +					   (u64)sbi->num_clusters << dentries_per_clu_bits);
> 
>  	clu_offset = dentry >> dentries_per_clu_bits;
>  	exfat_chain_dup(&clu, &dir);
> @@ -109,7 +111,7 @@ static int exfat_readdir(struct inode *inode, loff_t *cpos, struct exfat_dir_ent
>  		}
>  	}
> 
> -	while (clu.dir != EXFAT_EOF_CLUSTER) {
> +	while (clu.dir != EXFAT_EOF_CLUSTER && dentry < max_dentries) {
>  		i = dentry & (dentries_per_clu - 1);
> 
>  		for ( ; i < dentries_per_clu; i++, dentry++) { @@ -245,7 +247,7 @@ static int exfat_iterate(struct file *filp,
> struct dir_context *ctx)
>  	if (err)
>  		goto unlock;
>  get_new:
> -	if (cpos >= i_size_read(inode))
> +	if (ei->flags == ALLOC_NO_FAT_CHAIN && cpos >= i_size_read(inode))
>  		goto end_of_dir;

This check is no longer necessary.
In the case of ALLOC_NO_FAT_CHAIN, there is a check for i_size in exfat_readdir(), which is a double check.
In the past, it was possible to skip chain tracking in the case of ALLOC_FAT_CHAIN, but that effect is also gone.

> 
>  	err = exfat_readdir(inode, &cpos, &de);


BR
Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

Re: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Kohada.Tetsuhiro]

> This patch check DataLength in stream
> entry only if the type is ALLOC_NO_FAT_CHAIN and add the check ensure
> that dentry offset does not exceed max dentries size(256 MB) to avoid
> the circular FAT chain issue.

I think it's preferable to add a 256MB check during dir-scan.(as I said in the previous)
If you want to solve "the circular FAT chain issue", you should add it to other dir-scan loops.
(exfat_search_empty_slot, exfat_check_dir_empty, exfat_count_dir_entries, etc ...).

Also, the dir-scan loop may not terminate when TYPE_UNUSED is detected.
Even if TYPE_UNUSED is detected, just break the inner for-loop will continue the outer while-loop,
so the next cluster will be accessed.
If we can't use DataLength for checking, we should check the contents more strictly instead.

The current implementation has several similar dir-scan loops.
These are similar logics and should be abstracted, if possible.

BR
Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

RE: [PATCH] exfat: handle wrong stream entry size in exfat_readdir()

[Namjae Jeon]

> > This patch check DataLength in stream
> > entry only if the type is ALLOC_NO_FAT_CHAIN and add the check ensure
> > that dentry offset does not exceed max dentries size(256 MB) to avoid
> > the circular FAT chain issue.
> 
> I think it's preferable to add a 256MB check during dir-scan.(as I said in the previous) If you want
> to solve "the circular FAT chain issue", you should add it to other dir-scan loops.
> (exfat_search_empty_slot, exfat_check_dir_empty, exfat_count_dir_entries, etc ...).
> 
> Also, the dir-scan loop may not terminate when TYPE_UNUSED is detected.
> Even if TYPE_UNUSED is detected, just break the inner for-loop will continue the outer while-loop, so
> the next cluster will be accessed.
> If we can't use DataLength for checking, we should check the contents more strictly instead.
> 
> The current implementation has several similar dir-scan loops.
> These are similar logics and should be abstracted, if possible.
Can you send me the patch instead of just talking?

> 
> BR
> Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>