by Namjae Jeon

[permalink] [raw]

Subject: RE: [PATCH v3] exfat: integrates dir-entry getting and validation

> Thank you for quick reply!
>
> On 2020/08/26 13:19, Namjae Jeon wrote:
> >> On 2020/08/26 10:03, Namjae Jeon wrote:
> >>>> Second: Range validation and type validation should not be separated.
> >>>> When I started making this patch, I intended to add only range validation.
> >>>> However, after the caller gets the ep, the type validation follows.
> >>>> Get ep, null check of ep (= range verification), type verification is a series of procedures.
> >>>> There would be no reason to keep them independent anymore.
> >>>> Range and type validation is enforced when the caller uses ep.
> >>> You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict.
> >>> none : only range validation.
> >>> basic : range + type validation.
> >>> strict : range + type + checksum and name length, etc.
> >>
> >> Currently, various types of verification will not be needed.
> >> Let's add it when we need it.
> >>>
> >>>>> - /* validiate cached dentries */
> >>>>> - for (i = 1; i < num_entries; i++) {
> >>>>> - ep = exfat_get_dentry_cached(es, i);
> >>>>> - if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
> >>>>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM);
> >>>>> + if (!ep || ep->type != EXFAT_STREAM)
> >>>>> + goto free_es;
> >>>>> + es->de[ENTRY_STREAM] = ep;
> >>>>
> >>>> The value contained in stream-ext dir-entry should not be used
> >>>> before validating the EntrySet
> >> checksum.
> >>>> So I would insert EntrySet checksum validation here.
> >>>> In that case, the checksum verification loop would be followed by
> >>>> the TYPE_NAME verification loop, can you acceptable?
> >>> Yes. That would be great.
> >>
> >> OK.
> >> I'll add TYPE_NAME verification after checksum verification, in next patch.
> >> However, I think it is enough to validate TYPE_NAME when extracting name.
> >> Could you please tell me why you think you need TYPE_NAME validation here?
> > I've told you on previous mail. This function should return validated
> > dentry set after checking
> > file->stream->name in sequence.
>
> Yes. I understand that the current implementation checks in that order.
> Sorry, my question was unclear.
> Why do you think you should leave the TYPE_NAME validation in this function?
> What kind of problem are you worried about if this function does not validate TYPE_NAME?
> (for preserve the current behavior?)
We have not checked the problem when it is removed because it was implemented
according to the specification from the beginning. And your v3 patch are
already checking the name entries as TYPE_SECONDARY. And it check them with
TYPE_NAME again in exfat_get_uniname_from_ext_entry(). If you check TYPE_NAME
with stream->name_len, We don't need to perform the loop for extracting
filename from the name entries if stream->name_len or name entry is invalid.

And I request to prove why we do not need to validate name entries in this
function calling from somewhere. So as I suggested earlier, You can make it
with an argument flags so that we skip the validation.
>
> Don't worry, I will add TYPE_NAME verification to the v4 patch.
> I will post it later today.
Sound good.
>
> BR
> ---
> Tetsuhiro Kohada <[email protected]>

2020-08-27 10:03:49

by Tetsuhiro Kohada

[permalink] [raw]

Subject: Re: [PATCH v3] exfat: integrates dir-entry getting and validation

>>>>>>> - /* validiate cached dentries */
>>>>>>> - for (i = 1; i < num_entries; i++) {
>>>>>>> - ep = exfat_get_dentry_cached(es, i);
>>>>>>> - if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
>>>>>>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM);
>>>>>>> + if (!ep || ep->type != EXFAT_STREAM)
>>>>>>> + goto free_es;
>>>>>>> + es->de[ENTRY_STREAM] = ep;
>>>>>>
>>>>>> The value contained in stream-ext dir-entry should not be used
>>>>>> before validating the EntrySet
>>>> checksum.
>>>>>> So I would insert EntrySet checksum validation here.
>>>>>> In that case, the checksum verification loop would be followed by
>>>>>> the TYPE_NAME verification loop, can you acceptable?
>>>>> Yes. That would be great.
>>>>
>>>> OK.
>>>> I'll add TYPE_NAME verification after checksum verification, in next patch.
>>>> However, I think it is enough to validate TYPE_NAME when extracting name.
>>>> Could you please tell me why you think you need TYPE_NAME validation here?
>>> I've told you on previous mail. This function should return validated
>>> dentry set after checking
>>> file->stream->name in sequence.
>>
>> Yes. I understand that the current implementation checks in that order.
>> Sorry, my question was unclear.
>> Why do you think you should leave the TYPE_NAME validation in this function?
>> What kind of problem are you worried about if this function does not validate TYPE_NAME?
>> (for preserve the current behavior?)
> We have not checked the problem when it is removed because it was implemented
> according to the specification from the beginning.

I understand that the main reason to validate TYPE_NAME here is "according to the specification".
(No one knows the actual problem)

First, we should validate as 'dir-entry set' by SecondaryCount and SetChecksum described
in "6.3 Generic Primary DirectoryEntry Template".

Next, description about validity of 'File dir-entry set' is ...
7.4 File Directory Entry:
... For a File directory entry to be valid, exactly one Stream Extension directory entry and at least
one File Name directory entry must immediately follow the File directory entry.
7.7 File Name Directory Entry:
... File Name directory entries are valid only if they immediately follow the Stream Extension
directory entry as a consecutive series.

It is possible to validate the above correctly, with either exfat_get_dentry_set() or
exfat_get_uniname_from_name_entries().
Is this wrong?

> And your v3 patch are
> already checking the name entries as TYPE_SECONDARY. And it check them with
> TYPE_NAME again in exfat_get_uniname_from_ext_entry().

This is according to "6.3 Generic Primary DirectoryEntry Template".
"6.3 Generic Primary DirectoryEntry Template" only required TYPE_SECONDARY.
In v3, there is no checksum validation yet.

> If you check TYPE_NAME
> with stream->name_len, We don't need to perform the loop for extracting
> filename from the name entries if stream->name_len or name entry is invalid.

Don't worry, it's a rare case.
(Do you care about the run-time costs?)

> And I request to prove why we do not need to validate name entries in this
> function calling from somewhere.

If you need, it's okey to validate in both.
However, name-length and type validation and name-extraction should not be separated.
These are closely related, so these should be placed physically and temporally close.

Well, why it's unnecessary.
Both can be validate correctly, as I wrote before.
And, I don't really trust the verification with TYPE_NAME.
(reliability of validation as 'file dir-entry set' by checksum is much higher)

> So as I suggested earlier, You can make it
> with an argument flags so that we skip the validation.

No need skip the validation, I think.
The run-time costs for validation are pretty low.
The reason I want to remove the validation is because I want to keep the code simple.
(KISS principle)

BR
---
Tetsuhiro Kohada <[email protected]>