Re: [PATCH v2] exfat: integrates dir-entry getting and validation
+ i = 2;
+ while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
As Sungjong said, I think that TYPE_NAME seems right to be validated in
exfat_get_dentry_set().
First, it is possible to correctly determine that "Immediately follow the
Stream Extension directory
entry as a consecutive series"
whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
It's functionally same, so it is also right to validate in either.
Second, the current implementation does not care for NameLength field, as I
replied to Sungjong.
If name is not terminated with zero, the name will be incorrect.(With or
without my patch) I think
TYPE_NAME and NameLength validation should not be separated from the name
extraction.
If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name
extraction should also
be implemented there.
(Otherwise, a duplication check with exfat_get_dentry_set() and here.) I will
add NameLength
validation here.
Okay.
Thank you for your understanding.
Therefore, TYPE_NAME validation here should not be omitted.
Third, getting dentry and entry-type validation should be integrated.
These no longer have to be primitive.
The integration simplifies caller error checking.
diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
6707f3eb09b5..b6b458e6f5e3 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
ES_ALL_ENTRIES);
if (!es)
return -EIO;
- ep = exfat_get_dentry_cached(es, 0);
- ep2 = exfat_get_dentry_cached(es, 1);
+ ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
+ ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
TYPE_FILE and TYPE_STREAM was already validated in exfat_get_dentry_set().
Isn't it unnecessary duplication check ?
No, as you say.
Although TYPE is specified, it is not good not to check the null of ep/ep2.
However, with TYPE_ALL, it becomes difficult to understand what purpose ep/ep2
is used for.
Therefore, I proposed adding ep_file/ep_stream to es, and here
ep = es->ep_file;
ep2 = es->ep_stream;
How about this?
You can factor out exfat_get_dentry_cached() from exfat_get_validated_dentry()
and use it here.
I actually implemented and use it, but I feel it is not so good.
- Since there are two functions to get from es, so it's a bit confusing which
one is better for use.
- There was the same anxiety as using exfat_get_validated_dentry() in that
there is no NULL check of
ep got with exfat_get_dentry_cached().
Whichever function I use, there are places where I check the return value and
where I don't.
This will cause missing entry-type validation or missing return value check,in
the future.
I think it's easier to use by including it as a validated object in the member
of exfat_entry_set_cache.
And then, You can rename ep and ep2 to ep_file and ep_stream.
I propose a slightly different approach than last.
Add members to exfat_entry_set_cache as below.
struct exfat_de_file *de_file;
struct exfat_de_stream *de_stream;
And, use these as below.
es->de_file->attr = cpu_to_le16(exfat_make_attr(inode));
es->de_stream->valid_size = cpu_to_le64(on_disk_size);
exfat_de_file/exfat_de_stream corresponds to the file dir-entry/stream dir-enty
structure in the exfat_dentry union.
We can use the validated valid values directly.
Furthermore, these are strongly typed.
Or is it better to specify TYPE_ALL?
BTW
It's been about a month since I posted this patch.
In the meantime, I created a NameLength check and a checksum validation based
on this patch.
Can you review those as well?
Let me see the patches.
Thanks a lot.
For now, I will create and post a V3 patch with this proposal.
After that, I will recreate the NameLength check and a checksum validation
patches based on the V3 patch and post them.
(Should I post these as an RFC?)
BR
---
Kohada Tetsuhiro
RE: [PATCH v2] exfat: integrates dir-entry getting and validation
> Thank you for your reply.
>
> > > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > > 573659bfbc55..09b85746e760 100644
> > > --- a/fs/exfat/dir.c
> > > +++ b/fs/exfat/dir.c
> > > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct
> > > super_block *sb, {
> > > int i;
> > > struct exfat_entry_set_cache *es;
> > > + struct exfat_dentry *ep;
> > >
> > > es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > > if (!es)
> > > @@ -44,13 +45,9 @@ 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.
> > >*/
> > > - for (i = 2; i < es->num_entries; i++) {
> > > - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> > > -
> > > - /* end of name entry */
> > > - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> > > - break;
> > >
> > > + i = 2;
> > > + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> > As Sungjong said, I think that TYPE_NAME seems right to be validated in
> > exfat_get_dentry_set().
>
> First, it is possible to correctly determine that "Immediately follow the
> Stream Extension directory
> entry as a consecutive series"
> whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
> It's functionally same, so it is also right to validate in either.
>
> Second, the current implementation does not care for NameLength field, as I
> replied to Sungjong.
> If name is not terminated with zero, the name will be incorrect.(With or
> without my patch) I think
> TYPE_NAME and NameLength validation should not be separated from the name
> extraction.
> If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and
> name extraction should also
> be implemented there.
> (Otherwise, a duplication check with exfat_get_dentry_set() and here.) I will
> add NameLength
> validation here.
Okay.
> Therefore, TYPE_NAME validation here should not be omitted.
>
> Third, getting dentry and entry-type validation should be integrated.
> These no longer have to be primitive.
> The integration simplifies caller error checking.
>
>
> > > -struct exfat_dentry *exfat_get_dentry_cached(
> > > - struct exfat_entry_set_cache *es, int num)
> > > +struct exfat_dentry *exfat_get_validated_dentry(struct
> > > exfat_entry_set_cache *es,
> > > + int num, unsigned int type)
> > Please use two tabs.
>
> OK.
> I'll fix it.
>
>
> > > + struct buffer_head *bh;
> > > + struct exfat_dentry *ep;
> > >
> > > - return (struct exfat_dentry *)p;
> > > + if (num >= es->num_entries)
> > > + return NULL;
> > > +
> > > + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > > + if (!bh)
> > > + return NULL;
> > > +
> > > + ep = (struct exfat_dentry *)
> > > + (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > > +
> > > + switch (type) {
> > > + case TYPE_ALL: /* accept any */
> > > + break;
> > > + case TYPE_FILE:
> > > + if (ep->type != EXFAT_FILE)
> > > + return NULL;
> > > + break;
> > > + case TYPE_SECONDARY:
> > > + if (!(type & exfat_get_entry_type(ep)))
> > > + return NULL;
> > > + break;
> > Type check should be in this order :
> > FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> > I think that you are missing TYPE_NAME check here.
>
> Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the
> default-case(as below).
>
> > > + default:
> > > + if (type != exfat_get_entry_type(ep))
> > > + return NULL;
> > > + }
> > > + return ep;
> > > }
> > >
> > > /*
> > > * Returns a set of dentries for a file or dir.
> > > *
> > > - * Note It provides a direct pointer to bh->data via
> > > exfat_get_dentry_cached().
> > > + * Note It provides a direct pointer to bh->data via
> > > exfat_get_validated_dentry().
> > > * User should call exfat_get_dentry_set() after setting 'modified' to
> > > apply
> > > * changes made in this entry set to the real device.
> > > *
> > > * in:
> > > * sb+p_dir+entry: indicates a file/dir
> > > - * type: specifies how many dentries should be included.
> > > + * max_entries: specifies how many dentries should be included.
> > > * return:
> > > * pointer of entry set on success,
> > > * NULL on failure.
> > > + * note:
> > > + * On success, guarantee the correct 'file' and 'stream-ext'
> > > dir-entries.
> > This comment seems unnecessary.
>
> I'll remove it.
>
> > > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > > 6707f3eb09b5..b6b458e6f5e3 100644
> > > --- a/fs/exfat/file.c
> > > +++ b/fs/exfat/file.c
> > > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t
> > > new_size)
> > > ES_ALL_ENTRIES);
> > > if (!es)
> > > return -EIO;
> > > -
RE: [PATCH v2] exfat: integrates dir-entry getting and validation
Thank you for your reply.
> > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > 573659bfbc55..09b85746e760 100644
> > --- a/fs/exfat/dir.c
> > +++ b/fs/exfat/dir.c
> > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct
> > super_block *sb, {
> > int i;
> > struct exfat_entry_set_cache *es;
> > + struct exfat_dentry *ep;
> >
> > es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > if (!es)
> > @@ -44,13 +45,9 @@ 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.
> > */
> > - for (i = 2; i < es->num_entries; i++) {
> > - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> > -
> > - /* end of name entry */
> > - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> > - break;
> >
> > + i = 2;
> > + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> As Sungjong said, I think that TYPE_NAME seems right to be validated in
> exfat_get_dentry_set().
First, it is possible to correctly determine that
"Immediately follow the Stream Extension directory entry as a consecutive
series"
whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
It's functionally same, so it is also right to validate in either.
Second, the current implementation does not care for NameLength field, as I
replied to Sungjong.
If name is not terminated with zero, the name will be incorrect.(With or
without my patch)
I think TYPE_NAME and NameLength validation should not be separated from the
name extraction.
If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name
extraction
should also be implemented there.
(Otherwise, a duplication check with exfat_get_dentry_set() and here.)
I will add NameLength validation here.
Therefore, TYPE_NAME validation here should not be omitted.
Third, getting dentry and entry-type validation should be integrated.
These no longer have to be primitive.
The integration simplifies caller error checking.
> > -struct exfat_dentry *exfat_get_dentry_cached(
> > - struct exfat_entry_set_cache *es, int num)
> > +struct exfat_dentry *exfat_get_validated_dentry(struct
> > exfat_entry_set_cache *es,
> > + int num, unsigned int type)
> Please use two tabs.
OK.
I'll fix it.
> > + struct buffer_head *bh;
> > + struct exfat_dentry *ep;
> >
> > - return (struct exfat_dentry *)p;
> > + if (num >= es->num_entries)
> > + return NULL;
> > +
> > + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > + if (!bh)
> > + return NULL;
> > +
> > + ep = (struct exfat_dentry *)
> > + (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > +
> > + switch (type) {
> > + case TYPE_ALL: /* accept any */
> > + break;
> > + case TYPE_FILE:
> > + if (ep->type != EXFAT_FILE)
> > + return NULL;
> > + break;
> > + case TYPE_SECONDARY:
> > + if (!(type & exfat_get_entry_type(ep)))
> > + return NULL;
> > + break;
> Type check should be in this order :
> FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> I think that you are missing TYPE_NAME check here.
Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the
default-case(as below).
> > + default:
> > + if (type != exfat_get_entry_type(ep))
> > + return NULL;
> > + }
> > + return ep;
> > }
> >
> > /*
> > * Returns a set of dentries for a file or dir.
> > *
> > - * Note It provides a direct pointer to bh->data via
> > exfat_get_dentry_cached().
> > + * Note It provides a direct pointer to bh->data via
> > exfat_get_validated_dentry().
> > * User should call exfat_get_dentry_set() after setting 'modified' to
> > apply
> > * changes made in this entry set to the real device.
> > *
> > * in:
> > * sb+p_dir+entry: indicates a file/dir
> > - * type: specifies how many dentries should be included.
> > + * max_entries: specifies how many dentries should be included.
> > * return:
> > * pointer of entry set on success,
> > * NULL on failure.
> > + * note:
> > + * On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
> This comment seems unnecessary.
I'll remove it.
> > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > 6707f3eb09b5..b6b458e6f5e3 100644
> > --- a/fs/exfat/file.c
> > +++ b/fs/exfat/file.c
> > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t
> > new_size)
> > ES_ALL_ENTRIES);
> > if (!es)
> > return -EIO;
> > - ep = exfat_get_dentry_cached(es, 0);
> > - ep2 = exfat_get_dentry_cached(es, 1);
> > + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> > + ep2 =
RE: [PATCH v2] exfat: integrates dir-entry getting and validation
> Add validation for num, bh and type on getting dir-entry.
> ('file' and 'stream-ext' dir-entries are pre-validated to ensure success)
> Renamed
> exfat_get_dentry_cached() to exfat_get_validated_dentry() due to a change in
> functionality.
>
> Integrate type-validation with simplified.
> This will also recognize a dir-entry set that contains 'benign secondary'
> dir-entries.
>
> And, rename TYPE_EXTEND to TYPE_NAME.
>
> Suggested-by: Sungjong Seo
> Signed-off-by: Tetsuhiro Kohada
> ---
> Changes in v2
> - Change verification order
> - Verification loop start with index 2
>
> fs/exfat/dir.c | 144 ++--
> fs/exfat/exfat_fs.h | 15 +++--
> fs/exfat/file.c | 4 +-
> fs/exfat/inode.c| 6 +-
> fs/exfat/namei.c| 4 +-
> 5 files changed, 73 insertions(+), 100 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 573659bfbc55..09b85746e760
> 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb, {
> int i;
> struct exfat_entry_set_cache *es;
> + struct exfat_dentry *ep;
>
> es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> if (!es)
> @@ -44,13 +45,9 @@ 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.
>*/
> - for (i = 2; i < es->num_entries; i++) {
> - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> -
> - /* end of name entry */
> - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> - break;
>
> + i = 2;
> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
As Sungjong said, I think that TYPE_NAME seems right to be validated in
exfat_get_dentry_set().
> exfat_extract_uni_name(ep, uniname);
> uniname += EXFAT_FILE_NAME_LEN;
> }
> @@ -372,7 +369,7 @@ unsigned int exfat_get_entry_type(struct exfat_dentry *ep)
> if (ep->type == EXFAT_STREAM)
> return TYPE_STREAM;
> if (ep->type == EXFAT_NAME)
> - return TYPE_EXTEND;
> + return TYPE_NAME;
> if (ep->type == EXFAT_ACL)
> return TYPE_ACL;
> return TYPE_CRITICAL_SEC;
> @@ -388,7 +385,7 @@ static void exfat_set_entry_type(struct exfat_dentry *ep,
> unsigned int type)
> ep->type &= EXFAT_DELETE;
> } else if (type == TYPE_STREAM) {
> ep->type = EXFAT_STREAM;
> - } else if (type == TYPE_EXTEND) {
> + } else if (type == TYPE_NAME) {
> ep->type = EXFAT_NAME;
> } else if (type == TYPE_BITMAP) {
> ep->type = EXFAT_BITMAP;
> @@ -421,7 +418,7 @@ static void exfat_init_name_entry(struct exfat_dentry
> *ep, {
> int i;
>
> - exfat_set_entry_type(ep, TYPE_EXTEND);
> + exfat_set_entry_type(ep, TYPE_NAME);
> ep->dentry.name.flags = 0x0;
>
> for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) { @@ -594,12 +591,12 @@ void
> exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
> struct exfat_dentry *ep;
>
> for (i = 0; i < es->num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> + ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
> chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
>chksum_type);
> chksum_type = CS_DEFAULT;
> }
> - ep = exfat_get_dentry_cached(es, 0);
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> ep->dentry.file.checksum = cpu_to_le16(chksum);
> es->modified = true;
> }
> @@ -741,92 +738,66 @@ struct exfat_dentry *exfat_get_dentry(struct
> super_block *sb,
> return (struct exfat_dentry *)((*bh)->b_data + off); }
>
> -enum exfat_validate_dentry_mode {
> - ES_MODE_STARTED,
> - ES_MODE_GET_FILE_ENTRY,
> - ES_MODE_GET_STRM_ENTRY,
> - ES_MODE_GET_NAME_ENTRY,
> - ES_MODE_GET_CRITICAL_SEC_ENTRY,
> -};
> -
> -static bool exfat_validate_entry(unsigned int type,
> - enum exfat_validate_dentry_mode *mode)
> -{
> - if (type == TYPE_UNUSED || type == TYPE_DELETED)
> - return false;
> -
> - switch (*mode) {
> - case ES_MODE_STARTED:
> - if (type != TYPE_FILE && type != TYPE_DIR)
> - return false;
> - *mode = ES_MODE_GET_FILE_ENTRY;
> - return true;
> - case ES_MODE_GET_FILE_ENTRY:
> - if (type != TYPE_STREAM)
> - return false;
> - *mode = ES_MODE_GET_STRM_ENTRY;
> - return true;
> - case ES_MODE_GET_STRM_ENTRY:
> - if (type != TYPE_EXTEND)
> - return false;
> -
Re: [PATCH v2] exfat: integrates dir-entry getting and validation
On 2020/07/15 10:22, Tetsuhiro Kohada wrote:
Add validation for num, bh and type on getting dir-entry.
('file' and 'stream-ext' dir-entries are pre-validated to ensure success)
Renamed exfat_get_dentry_cached() to exfat_get_validated_dentry() due to
a change in functionality.
Integrate type-validation with simplified.
This will also recognize a dir-entry set that contains 'benign secondary'
dir-entries.
And, rename TYPE_EXTEND to TYPE_NAME.
Suggested-by: Sungjong Seo
Signed-off-by: Tetsuhiro Kohada
---
Changes in v2
- Change verification order
- Verification loop start with index 2
Ping.
Is there anything I should do?
BR
---
Tetsuhiro Kohada
