| Message ID | 20200806055653.9329-1-kohada.t2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] exfat: add NameLength check when extracting name | expand |
> 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@gmail.com> > --- > 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. > > /* > * 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. > 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
> 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@gmail.com> > --- > 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; Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ? > > /* > * 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]); > } > - > - exfat_free_dentry_set(es, false); > + uniname->name[l] = 0; > + return 0; > }
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@gmail.com> >> --- >> 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 > >
Thank you for your reply. >> -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; > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ? Yes. As I wrote in a previous email, entry type validation, name-length validation, and name extraction should not be separated, so implement all of these in exfat_get_dentry_set(). It can be easily implemented by adding uniname to exfat_entry_set_cache and calling exfat_get_uniname_from_name_entries() from exfat_get_dentry_set(). However, that would be over-implementation. Not all callers of exfat_get_dentry_set() need a name. It is enough to validate the name when it is needed. This is a file-system driver, not fsck. Validation is possible in exfat_get_dentry_set(), but unnecessary. Why do you want to validate the name in exfat_get_dentry_set()? BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> Thank you for your reply. > > >> -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; > > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ? > > Yes. > As I wrote in a previous email, entry type validation, name-length validation, and name extraction > should not be separated, so implement all of these in exfat_get_dentry_set(). > It can be easily implemented by adding uniname to exfat_entry_set_cache and calling > exfat_get_uniname_from_name_entries() from exfat_get_dentry_set(). No, We can check stream->name_len and name entry type in exfat_get_dentry_set(). And you are already checking entry type with TYPE_SECONDARY in exfat_get_dentry_set(). Why do we have to check twice? > > However, that would be over-implementation. > Not all callers of exfat_get_dentry_set() need a name. Where? It will not checked with ES_2_ENTRIES. > It is enough to validate the name when it is needed. > This is a file-system driver, not fsck. Sorry, I don't understand what you are talking about. If there is a problem in ondisk-metadata, Filesystem should return error. > Validation is possible in exfat_get_dentry_set(), but unnecessary. > > Why do you want to validate the name in exfat_get_dentry_set()? exfat_get_dentry_set validates file, stream entry. And you are trying to check name entries with type_secondary. In addition, trying add the checksum check. Conversely, Why would you want to add those checks to exfat_get_dentry_set()? Why do we check only name entries separately? Aren't you intent to return validated entry set in exfat_get_dentry_set()? > > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com>
Thank you for your reply. > > >> -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; > > > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ? > > > > Yes. > > As I wrote in a previous email, entry type validation, name-length > > validation, and name extraction should not be separated, so implement all of these in exfat_get_dentry_set(). > > It can be easily implemented by adding uniname to > > exfat_entry_set_cache and calling > > exfat_get_uniname_from_name_entries() from exfat_get_dentry_set(). > No, We can check stream->name_len and name entry type in exfat_get_dentry_set(). I have no objection to that point. > And you are already checking entry type with TYPE_SECONDARY in exfat_get_dentry_set(). Why do we have to check twice? This verification is according to the description in '6.3 Generic Primary DirectoryEntry Template'. The EntrySet is composed of one primary dir-entry and multiple Secondary dir-entries. We can validate the EntrySet by SecondaryCount and SetChecksum recorded in the Primary dir-entry. The EntrySet checksum validation and dir-entries order validation are according to different descriptions. Therefore, it is not a double check. > > However, that would be over-implementation. > > Not all callers of exfat_get_dentry_set() need a name. > Where? It will not checked with ES_2_ENTRIES. The following functions don't require name. __exfat_truncate() __exfat_write_inode() exfat_map_cluster() exfat_find() > > It is enough to validate the name when it is needed. > > This is a file-system driver, not fsck. > Sorry, I don't understand what you are talking about. If there is a problem in ondisk-metadata, Filesystem should return > error. My explanation may have been inappropriate. (Verifier is a better metaphor than fsck) Essentially, the purpose of file-system driver is not to detect inconsistencies. Of course, FSD should return error when it detects an inconsistency, as you say. However, I think it is no-need for active inconsistency detection. > > Validation is possible in exfat_get_dentry_set(), but unnecessary. > > > > Why do you want to validate the name in exfat_get_dentry_set()? > exfat_get_dentry_set validates file, stream entry. > And you are trying to check name entries with type_secondary. It's a little different. I'm trying to validate the checksum according to '6.3 Generic Primary DirectoryEntry Template'. > In addition, trying add the checksum check. > Conversely, Why would you want to add those checks to exfat_get_dentry_set()? We should validate the EntrySet before using its contents. (should not use contents of the EntrySet without validating it) There are other filesystem designs that include crc/checksum in their each metadata headers. Such a design can detect inconsistencies easily and effectively. This verification is especially effective when meta-data is recorded in multiple sectors like the EntrySet. > Why do we check only name entries separately? This verification is according to the description in '7.6.3 NameLength Field' and '7.7 File Name Directory Entry'. Separated because it is according to different description from checksum. As I wrote before, I think TYPE_NAME and NameLength validation should not be separated from the name extraction. (Because they are more strongly related than order of dir-entries) So I think these should not be mixed with checksum verification. When packing names into Name Dir-Entry... We can also calculate the checksum while packing the name into the Name Dir-Entry. However, we shouldn't mix them. > Aren't you intent to return validated entry set in exfat_get_dentry_set()? If so, add exfat_get_uniname_from_name_entries() after checksum verification.(as below) ---------------------------------------------------- /* EntrySet checksum verification */ + if (max_entries == ES_ALL_ENTRIES && + exfat_get_uniname_from_name_entries(es, es->uniname)) + goto free_es; return es; ---------------------------------------------------- The only callers that need a name are exfat_readdir() and exfat_find_dir_entry(), not the others. Unnecessary name extraction violates the JIT principle. (The size of exfat_entry_set_cache will also increase) And even if we call exfat_get_uniname_from_name_entries() later, we can correctly determine "File Name directory entries are valid only if they immediately follow the Stream Extension directory entry as a consecutive series" File dir-entry set can contain dir-entries other than file, stream-ext and name. We don't need to validate or extract them all in exfat_get_dentry_set(). I think exfat_entry_set_cache only needs to provide a framework for easy access when needed. BR --- Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>
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; /* * 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]); } - - 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) sector_t sector; struct exfat_chain dir, clu; struct exfat_uni_name uni_name; + struct exfat_entry_set_cache *es; struct exfat_dentry *ep; struct super_block *sb = inode->i_sb; struct exfat_sb_info *sbi = EXFAT_SB(sb); @@ -114,47 +115,43 @@ static int exfat_readdir(struct inode *inode, struct exfat_dir_entry *dir_entry) return -EIO; type = exfat_get_entry_type(ep); - if (type == TYPE_UNUSED) { - brelse(bh); + brelse(bh); + + if (type == TYPE_UNUSED) break; - } - if (type != TYPE_FILE && type != TYPE_DIR) { - brelse(bh); + if (type != TYPE_FILE && type != TYPE_DIR) continue; - } - dir_entry->attr = le16_to_cpu(ep->dentry.file.attr); + es = exfat_get_dentry_set(sb, &dir, dentry, ES_ALL_ENTRIES); + if (!es) + return -EIO; + + dir_entry->attr = le16_to_cpu(es->de_file->attr); exfat_get_entry_time(sbi, &dir_entry->crtime, - ep->dentry.file.create_tz, - ep->dentry.file.create_time, - ep->dentry.file.create_date, - ep->dentry.file.create_time_cs); + es->de_file->create_tz, + es->de_file->create_time, + es->de_file->create_date, + es->de_file->create_time_cs); exfat_get_entry_time(sbi, &dir_entry->mtime, - ep->dentry.file.modify_tz, - ep->dentry.file.modify_time, - ep->dentry.file.modify_date, - ep->dentry.file.modify_time_cs); + es->de_file->modify_tz, + es->de_file->modify_time, + es->de_file->modify_date, + es->de_file->modify_time_cs); exfat_get_entry_time(sbi, &dir_entry->atime, - ep->dentry.file.access_tz, - ep->dentry.file.access_time, - ep->dentry.file.access_date, + es->de_file->access_tz, + es->de_file->access_time, + es->de_file->access_date, 0); - *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); 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;
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@gmail.com> --- fs/exfat/dir.c | 81 ++++++++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 42 deletions(-)