Re: [PATCH 1/2] exfat: add NameLength check when extracting name

[Tetsuhiro Kohada]

Thanks for your reply.

On 2020/08/09 1:54, Sungjong Seo wrote:
> > The current implementation doesn't care NameLength when extracting the
> > name from Name dir-entries, so the name may be incorrect.
> > (Without null-termination, Insufficient Name dir-entry, etc) Add a
> > NameLength check when extracting the name from Name dir-entries to extract
> > correct name.
> > And, change to get the information of file/stream-ext dir-entries via the
> > member variable of exfat_entry_set_cache.
> >
> > ** This patch depends on:
> >    '[PATCH v3] exfat: integrates dir-entry getting and validation'.
> >
> > Signed-off-by: Tetsuhiro Kohada <kohada.t2@xxxxxxxxx>
> > ---
> >   fs/exfat/dir.c | 81 ++++++++++++++++++++++++--------------------------
> >   1 file changed, 39 insertions(+), 42 deletions(-)
> >
> > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > 91cdbede0fd1..545bb73b95e9 100644
> > --- a/fs/exfat/dir.c
> > +++ b/fs/exfat/dir.c
> > @@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry
> > *ep,
> >
> >   }
> >
> > -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> > -		struct exfat_chain *p_dir, int entry, unsigned short
> > *uniname)
> > +static int exfat_get_uniname_from_name_entries(struct
> > exfat_entry_set_cache *es,
> > +		struct exfat_uni_name *uniname)
> >   {
> > -	int i;
> > -	struct exfat_entry_set_cache *es;
> > +	int n, l, i;
> >   	struct exfat_dentry *ep;
> >
> > -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > -	if (!es)
> > -		return;
> > +	uniname->name_len = es->de_stream->name_len;
> > +	if (uniname->name_len == 0)
> > +		return -EIO;
>
> -EINVAL looks better.

OK.
I'll change in v2.

> >   	/*
> >   	 * First entry  : file entry
> > @@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct
> > super_block *sb,
> >   	 * Third entry  : first file-name entry
> >   	 * So, the index of first file-name dentry should start from 2.
> >   	 */
> > -
> > -	i = 2;
> > -	while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> > -		exfat_extract_uni_name(ep, uniname);
> > -		uniname += EXFAT_FILE_NAME_LEN;
> > +	for (l = 0, n = 2; l < uniname->name_len; n++) {
> > +		ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
> > +		if (!ep)
> > +			return -EIO;
> > +		for (i = 0; l < uniname->name_len && i <
> EXFAT_FILE_NAME_LEN;
> > i++, l++)
> > +			uniname->name[l] = le16_to_cpu(ep-
> > > dentry.name.unicode_0_14[i]);
>
> Looks good.
>
> >   	}
> > -
> > -	exfat_free_dentry_set(es, false);
> > +	uniname->name[l] = 0;
> > +	return 0;
> >   }
> >
> >   /* read a directory entry from the opened directory */ @@ -63,6 +63,7 @@
> > static int exfat_readdir(struct inode *inode, struct exfat_dir_entry
> > *dir_entry)
> [snip]
> > -			*uni_name.name = 0x0;
> > -			exfat_get_uniname_from_ext_entry(sb, &dir, dentry,
> > -				uni_name.name);
> > +			dir_entry->size = le64_to_cpu(es->de_stream-
> > > valid_size);
> > +
> > +			exfat_get_uniname_from_name_entries(es, &uni_name);
>
> Modified function has a return value.
> It would be better to check the return value.

Oops!
I'll fix it in v2.


> >   			exfat_utf16_to_nls(sb, &uni_name,
> >   				dir_entry->namebuf.lfn,
> >   				dir_entry->namebuf.lfnbuf_len);
> > -			brelse(bh);
> >
> > -			ep = exfat_get_dentry(sb, &clu, i + 1, &bh, NULL);
> > -			if (!ep)
> > -				return -EIO;
> > -			dir_entry->size =
> > -				le64_to_cpu(ep->dentry.stream.valid_size);
> > -			brelse(bh);
> > +			exfat_free_dentry_set(es, false);
> >
> >   			ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
> >   			ei->hint_bmap.clu = clu.dir;
> > --
> > 2.25.1