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:     <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>
Cc:     <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: <TY2PR01MB287579A95A7994DE2B34E425904D0@TY2PR01MB2875.jpnprd01.prod.outlook.com>
Subject: RE: [PATCH v2] exfat: integrates dir-entry getting and validation
Date:   Tue, 4 Aug 2020 10:28:22 +0900
Message-ID: <001c01d669fe$8ab7cf80$a0276e80$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Thread-Index: AQJkKvf3lVanaAlN2afWcUIlbNXqzwJnQ0rYAkopOF4BRzP236fb23SA
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20200715012304epcas1p23e9f45415afc551beea122e4e1bdb933@epcas1p2.samsung.com>
        <20200715012249.16378-1-kohada.t2@gmail.com>
        <015d01d6663e$1eb8c780$5c2a5680$@samsung.com>
        <TY2PR01MB287579A95A7994DE2B34E425904D0@TY2PR01MB2875.jpnprd01.prod.outlook.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> 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;
> > > -		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.
And then, You can rename ep and ep2 to ep_file and ep_stream.
> 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!
> 
> BR
> ---
> Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>