Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Namjae Jeon" <namjae.jeon@samsung.com>
To:     "'Tetsuhiro Kohada'" <kohada.t2@gmail.com>
Cc:     <kohada.tetsuhiro@dc.mitsubishielectric.co.jp>,
        <mori.takahiro@ab.mitsubishielectric.co.jp>,
        <motai.hirotaka@aj.mitsubishielectric.co.jp>,
        "'Sungjong Seo'" <sj1557.seo@samsung.com>,
        <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
In-Reply-To: <7d7ec460-b5ab-68da-658b-2104f393b4e8@gmail.com>
Subject: RE: [PATCH v3] exfat: integrates dir-entry getting and validation
Date:   Wed, 26 Aug 2020 16:32:04 +0900
Message-ID: <004301d67b7a$ff0dcf50$fd296df0$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Thread-Index: AQH6G9isXEQ9sMxkt33jRW4ItYjr3QIF0gSvAfjtwRwBp6Lp0QLf0PstAjwGXzkCQUE62QDGw52qAUD18EYBLf0H56iA6BpA
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20200806010250epcas1p482847d6d906fbf0ccd618c7d1cacd12e@epcas1p4.samsung.com>
        <20200806010229.24690-1-kohada.t2@gmail.com>
        <003c01d66edc$edbb1690$c93143b0$@samsung.com>
        <ca3b2b52-1abc-939c-aa11-8c7d12e4eb2e@gmail.com>
        <000001d67787$d3abcbb0$7b036310$@samsung.com>
        <fdaff3a3-99ba-8b9e-bdaf-9bcf9d7208e0@gmail.com>
        <000101d67b44$ac458c80$04d0a580$@samsung.com>
        <d1df9cca-3020-9e1e-0f3d-9db6752a22b6@gmail.com>
        <002e01d67b60$0b7d82a0$227887e0$@samsung.com>
        <7d7ec460-b5ab-68da-658b-2104f393b4e8@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> 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 <kohada.t2@gmail.com>