On Tue, Jun 17, 2025 at 2:38 PM Yuezhang.Mo@xxxxxxxx
<Yuezhang.Mo@xxxxxxxx> wrote:
>
> > On Fri, Jun 13, 2025 at 7:39 PM Yuezhang Mo <Yuezhang.Mo@xxxxxxxx> wrote:
> > >
> > > An infinite loop may occur if the following conditions occur due to
> > > file system corruption.
> > >
> > > (1) Condition for exfat_count_dir_entries() to loop infinitely.
> > > - The cluster chain includes a loop.
> > > - There is no UNUSED entry in the cluster chain.
> > >
> > > (2) Condition for exfat_create_upcase_table() to loop infinitely.
> > > - The cluster chain of the root directory includes a loop.
> > > - There are no UNUSED entry and up-case table entry in the cluster
> > > chain of the root directory.
> > >
> > > (3) Condition for exfat_load_bitmap() to loop infinitely.
> > > - The cluster chain of the root directory includes a loop.
> > > - There are no UNUSED entry and bitmap entry in the cluster chain
> > > of the root directory.
> > >
> > > This commit adds checks in exfat_count_num_clusters() and
> > > exfat_count_dir_entries() to see if the cluster chain includes a loop,
> > > thus avoiding the above infinite loops.
> > >
> > > Signed-off-by: Yuezhang Mo <Yuezhang.Mo@xxxxxxxx>
> > > ---
> > > fs/exfat/dir.c | 33 +++++++++++++++++++++------------
> > > fs/exfat/fatent.c | 10 ++++++++++
> > > fs/exfat/super.c | 32 +++++++++++++++++++++-----------
> > > 3 files changed, 52 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
> > > index 3103b932b674..467271ad4d71 100644
> > > --- a/fs/exfat/dir.c
> > > +++ b/fs/exfat/dir.c
> > > @@ -1194,7 +1194,8 @@ int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir)
> > > {
> > > int i, count = 0;
> > > int dentries_per_clu;
> > > - unsigned int entry_type;
> > > + unsigned int entry_type = TYPE_FILE;
> > > + unsigned int clu_count = 0;
> > > struct exfat_chain clu;
> > > struct exfat_dentry *ep;
> > > struct exfat_sb_info *sbi = EXFAT_SB(sb);
> > > @@ -1205,18 +1206,26 @@ int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir)
> > > exfat_chain_dup(&clu, p_dir);
> > >
> > > while (clu.dir != EXFAT_EOF_CLUSTER) {
> > > - for (i = 0; i < dentries_per_clu; i++) {
> > > - ep = exfat_get_dentry(sb, &clu, i, &bh);
> > > - if (!ep)
> > > - return -EIO;
> > > - entry_type = exfat_get_entry_type(ep);
> > > - brelse(bh);
> > > + clu_count++;
> > > + if (clu_count > sbi->used_clusters) {
> > if (++clu_count > sbi->used_clusters) {
>
> Well, that's more concise.
>
> > > + exfat_fs_error(sb, "dir size or FAT or bitmap is corrupted");
> > > + return -EIO;
> > > + }
> > >
> > > - if (entry_type == TYPE_UNUSED)
> > > - return count;
> > > - if (entry_type != TYPE_DIR)
> > > - continue;
> > > - count++;
> > > + if (entry_type != TYPE_UNUSED) {
> > > + for (i = 0; i < dentries_per_clu; i++) {
> > > + ep = exfat_get_dentry(sb, &clu, i, &bh);
> > > + if (!ep)
> > > + return -EIO;
> > > + entry_type = exfat_get_entry_type(ep);
> > > + brelse(bh);
> > > +
> > > + if (entry_type == TYPE_UNUSED)
> > > + break;
> > Is there any reason why you keep doing loop even though you found an
> > unused entry?
>
> There are unused directory entries when calling this func, but there
> may be none after files are created. That will cause a infinite loop in
> exfat_check_dir_empty() and exfat_find_dir_entry().
Isn't the infinite loop issue in this patch improved by checking
clu_count and ->used_clusters?
If it reaches an unused entry, how about returning right away like before?
Thanks.
>
> >
> > > + if (entry_type != TYPE_DIR)
> > > + continue;
> > > + count++;
> > > + }
> > > }
> > >
> > > if (clu.flags == ALLOC_NO_FAT_CHAIN) {
> > > diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
> > > index 23065f948ae7..2a2615ca320f 100644
> > > --- a/fs/exfat/fatent.c
> > > +++ b/fs/exfat/fatent.c
> > > @@ -490,5 +490,15 @@ int exfat_count_num_clusters(struct super_block *sb,
> > > }
> > >
> > > *ret_count = count;
> > > +
> > > + /*
> > > + * since exfat_count_used_clusters() is not called, sbi->used_clusters
> > > + * cannot be used here.
> > > + */
> > > + if (i == sbi->num_clusters) {
> > This is also right, But to make it more clear, wouldn't it be better
> > to do clu != EXFAT_EOF_CLUSTER?
>
> OK, I will add this.
>
> >
> > Thanks.
> > > + exfat_fs_error(sb, "The cluster chain has a loop");
> > > + return -EIO;
> > > + }
> > > +
> > > return 0;
> > > }
>
