| 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
| ||
|
Message-ID: <TY2PR01MB287579A95A7994DE2B34E425904D0@TY2PR01MB2875.jpnprd01.prod.outlook.com>
Date: Mon, 3 Aug 2020 07:31:02 +0000
From: "Kohada.Tetsuhiro@...MitsubishiElectric.co.jp"
<Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>
To: 'Namjae Jeon' <namjae.jeon@...sung.com>
CC: "Mori.Takahiro@...MitsubishiElectric.co.jp"
<Mori.Takahiro@...MitsubishiElectric.co.jp>,
"Motai.Hirotaka@...MitsubishiElectric.co.jp"
<Motai.Hirotaka@...MitsubishiElectric.co.jp>,
'Sungjong Seo' <sj1557.seo@...sung.com>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"linux-kernel@...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.
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?
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?
BR
---
Kohada Tetsuhiro <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>
Powered by blists - more mailing lists