linux-kernel - RE: RE: [PATCH v1] exfat: remove the code that sets FileAttributes when renaming

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <08bb01d8c823$ed1e0dd0$c75a2970$@samsung.com>
Date:   Wed, 14 Sep 2022 19:22:43 +0900
From:   "Sungjong Seo" <sj1557.seo@...sung.com>
To:     "'Rover'" <739817562@...com>,
        "'linkinjeon'" <linkinjeon@...nel.org>
Cc:     "'linux-kernel'" <linux-kernel@...r.kernel.org>,
        "'linux-fsdevel'" <linux-fsdevel@...r.kernel.org>,
        <sj1557.seo@...sung.com>
Subject: RE: RE: [PATCH v1] exfat: remove the code that sets FileAttributes
 when renaming

> Hi Sungjong,
> 
> This patch is to remove the duplicate setting ATTR_ARCHIVE when renaming.
> 
> We know that a file will not become a directory after being renamed, and a
> directory will not become a file after being renamed.

Sure, rename-op does not change any types of files.

> &gt;&gt;
> &gt;&gt; 		*epnew = *epold;
> So ATTR_ARCHIVE is already set here if rename file.

Do you think ATTR_ARCHIVE represents the file type? If so, you probably
are misunderstanding ATTR_ARCHIVE. It's just like a flag indicating that
there was a change. For example, in case of rename, it's just like a flag
indicating that its name has been changed.

The current linux exfat implementation does not yet provide an IOCTL
to control this attribute, but keep in mind that the ATTR_ARCHIVE may
be removed even if it is a file via another exfat driver.

B.R.
Sungjong Seo

> 
> &gt;&gt; -		if (exfat_get_entry_type(epnew) == TYPE_FILE) {
> &gt;&gt; -			epnew-&gt;dentry.file.attr |=
> cpu_to_le16(ATTR_ARCHIVE);
> No need to set again here.
> 
> &gt;&gt; -			ei-&gt;attr |= ATTR_ARCHIVE;
> ei-&gt;attr doesn't change and already set ATTR_ARCHIVE in
> exfat_fill_inode()
> 
> &gt;&gt; -		}
> 
> Best regards,
> Rover
> 
> ------------------ Original ------------------
> From: "Sungjong Seo" <sj1557.seo@...sung.com>;
> Date: Wed, Sep 14, 2022 04:48 PM
> To: "Rover"&lt;739817562@...com&gt;;"'linkinjeon'"<linkinjeon@...nel.org>;
> Cc: "'linux-kernel'"<linux-kernel@...r.kernel.org>;"'linux-
> fsdevel'"<linux-
> fsdevel@...r.kernel.org>;"sj1557.seo"<sj1557.seo@...sung.com>;
> Subject: RE: [PATCH v1] exfat: remove the code that sets FileAttributes
> when renaming
> 
> 
> Hi, Rover,
> 
> This patch seems to violate the exFAT specification below.
> Please refer to the description for ATTR_ARCHIVE in FAT32 Spec.
> 
> * Archive
> This field is mandatory and conforms to the MS-DOS definition.
> 
> * ATTR_ARCHIVE
> This attribute supports backup utilities. This bit is set by the FAT file
> system driver when a file is created, renamed, or written to. Backup
> utilities may use this attribute to indicate which files on the volume
> have been modified since the last time that a backup was performed.
> 
> Thanks.
> B.R.
> 
> Sungjong Seo
> 
> &gt; When renaming, FileAttributes remain unchanged, do not need to be
&gt;
> set, so the code that sets FileAttributes is unneeded, remove it.
> &gt;
> &gt; Signed-off-by: rover &amp;lt;739817562@...com&amp;gt; &gt; ---
> &gt;&nbsp; fs/exfat/namei.c | 12 ------------ &gt;&nbsp; 1 file changed,
> 12 deletions(-) &gt; &gt; diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> &gt; index b617bebc3d0f..5ffaf553155e 100644 &gt; --- a/fs/exfat/namei.c
> &gt; +++ b/fs/exfat/namei.c &gt; @@ -1031,10 +1031,6 @@ static int
> exfat_rename_file(struct inode *inode, &gt; struct exfat_chain *p_dir,
> &gt;&nbsp;return -EIO; &gt; &gt;&nbsp; *epnew = *epold; &gt; -if
> (exfat_get_entry_type(epnew) == TYPE_FILE) { &gt; - epnew-
> &amp;gt;dentry.file.attr |= &gt; cpu_to_le16(ATTR_ARCHIVE); &gt; - ei-
> &amp;gt;attr |= ATTR_ARCHIVE; &gt; -} &gt;&nbsp; exfat_update_bh(new_bh,
> sync); &gt;&nbsp; brelse(old_bh); &gt;&nbsp; brelse(new_bh); &gt; @@ -
> 1063,10 +1059,6 @@ static int exfat_rename_file(struct inode *inode, &gt;
> struct exfat_chain *p_dir, &gt;&nbsp; ei-&amp;gt;dir = *p_dir; &gt;&nbsp;
> ei-&amp;gt;entry = newentry; &gt;&nbsp;} else { &gt; -if
> (exfat_get_entry_type(epold) == TYPE_FILE) { &gt; - epold-
> &amp;gt;dentry.file.attr |= &gt; cpu_to_le16(ATTR_ARCHIVE); &gt; - ei-
> &amp;gt;attr |= ATTR_ARCHIVE; &gt; -} &gt;&nbsp; exfat_update_bh(old_bh,
> sync); &gt;&nbsp; brelse(old_bh); &gt;&nbsp; ret =
> exfat_init_ext_entry(inode, p_dir, oldentry, &gt; @@ -1112,10 +1104,6 @@
> static int exfat_move_file(struct inode *inode, &gt; struct exfat_chain
> *p_olddir, &gt;&nbsp; return -EIO; &gt; &gt;&nbsp;*epnew = *epmov; &gt; -
> if (exfat_get_entry_type(epnew) == TYPE_FILE) { &gt; -epnew-
> &amp;gt;dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); &gt; -ei-
> &amp;gt;attr |= ATTR_ARCHIVE; &gt; - } &gt;&nbsp;exfat_update_bh(new_bh,
> IS_DIRSYNC(inode)); &gt;&nbsp;brelse(mov_bh); &gt;&nbsp;brelse(new_bh);
> &gt; -- &gt; 2.25.1</sj1557.seo@...sung.com></linux-
> fsdevel@...r.kernel.org></linux-
> kernel@...r.kernel.org></linkinjeon@...nel.org></sj1557.seo@...sung.com>