| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Aug 2020 10:28:22 +0900
From: "Namjae Jeon" <namjae.jeon@...sung.com>
To: <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>
Cc: <Mori.Takahiro@...MitsubishiElectric.co.jp>,
<Motai.Hirotaka@...MitsubishiElectric.co.jp>,
"'Sungjong Seo'" <sj1557.seo@...sung.com>,
<linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: 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;
> > > - 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@...MitsubishiElectric.co.jp>
Powered by blists - more mailing lists