| 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: <000001dc1a5c$aaac6350$000529f0$@samsung.com>
Date: Sun, 31 Aug 2025 18:50:21 +0900
From: "Sungjong Seo" <sj1557.seo@...sung.com>
To: "'Ethan Ferguson'" <ethan.ferguson@...ier.com>, <linkinjeon@...nel.org>,
<yuezhang.mo@...y.com>
Cc: <linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<cpgs@...sung.com>
Subject: RE: [PATCH v4 1/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL
Hi,
> Add support for reading / writing to the exfat volume label from the
> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
>
> Signed-off-by: Ethan Ferguson <ethan.ferguson@...ier.com>
> ---
> fs/exfat/exfat_fs.h | 5 +
> fs/exfat/exfat_raw.h | 6 ++
> fs/exfat/file.c | 88 +++++++++++++++++
> fs/exfat/super.c | 224 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 323 insertions(+)
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index f8ead4d47ef0..ed4b5ecb952b 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -267,6 +267,7 @@ struct exfat_sb_info {
> struct buffer_head **vol_amap; /* allocation bitmap */
>
> unsigned short *vol_utbl; /* upcase table */
> + unsigned short *volume_label; /* volume name */
Why is it necessary to allocate and cache it? I didn't find where to reuse
it.
Is there a reason why uniname is not used directly as an argument?
Is there something I missed?
>
> unsigned int clu_srch_ptr; /* cluster search pointer */
> unsigned int used_clusters; /* number of used clusters */
> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
> inode *inode)
[snip]
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 538d2b6ac2ec..970e3ee57c43 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -12,6 +12,7 @@
> #include <linux/security.h>
> #include <linux/msdos_fs.h>
> #include <linux/writeback.h>
> +#include "../nls/nls_ucs2_utils.h"
>
> #include "exfat_raw.h"
> #include "exfat_fs.h"
> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
> *sb, unsigned long arg)
> return exfat_force_shutdown(sb, flags);
> }
>
> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
> long arg)
> +{
> + int ret;
> + char utf8[FSLABEL_MAX] = {0};
> + struct exfat_uni_name *uniname;
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +
> + uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
> + if (!uniname)
> + return -ENOMEM;
> +
> + ret = exfat_read_volume_label(sb);
> + if (ret < 0)
> + goto cleanup;
> +
> + memcpy(uniname->name, sbi->volume_label,
> + EXFAT_VOLUME_LABEL_LEN * sizeof(short));
> + uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
> + uniname->name_len = UniStrnlen(uniname->name,
> EXFAT_VOLUME_LABEL_LEN);
The volume label length is stored on-disk. It makes sense to retrieve
it directly. This way, there is no need to unnecessarily include the
NLS utility header file.
> +
> + ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
> + if (ret < 0)
> + goto cleanup;
> +
> + if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
> + ret = -EFAULT;
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + kfree(uniname);
> + return ret;
> +}
> +
> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
> + unsigned long arg,
> + struct inode *root_inode)
> +{
> + int ret, lossy;
> + char utf8[FSLABEL_MAX];
> + struct exfat_uni_name *uniname;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
> + if (!uniname)
> + return -ENOMEM;
> +
> + if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
> + ret = -EFAULT;
> + goto cleanup;
> + }
> +
> + if (utf8[0]) {
> + ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
> FSLABEL_MAX),
> + uniname, &lossy);
> + if (ret < 0)
> + goto cleanup;
> + else if (lossy & NLS_NAME_LOSSY) {
> + ret = -EINVAL;
> + goto cleanup;
> + }
> + } else {
> + uniname->name[0] = 0x0000;
> + uniname->name_len = 0;
> + }
> +
> + if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
> + exfat_info(sb, "Volume label length too long, truncating");
> + uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
> + }
> +
> + ret = exfat_write_volume_label(sb, uniname, root_inode);
> +
> +cleanup:
> + kfree(uniname);
> + return ret;
> +}
> +
> long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> struct inode *inode = file_inode(filp);
> u32 __user *user_attr = (u32 __user *)arg;
> + struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
>From this point, there is no need to pass root_inode. The root_inode can be
obtained directly from sb->s_root->d_inode within the function.
>
> switch (cmd) {
> case FAT_IOCTL_GET_ATTRIBUTES:
> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
> return exfat_ioctl_shutdown(inode->i_sb, arg);
> case FITRIM:
> return exfat_ioctl_fitrim(inode, arg);
> + case FS_IOC_GETFSLABEL:
> + return exfat_ioctl_get_volume_label(inode->i_sb, arg);
> + case FS_IOC_SETFSLABEL:
> + return exfat_ioctl_set_volume_label(inode->i_sb, arg,
> root_inode);
> default:
> return -ENOTTY;
> }
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 8926e63f5bb7..7931cdb4a1d1 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -18,6 +18,7 @@
> #include <linux/nls.h>
> #include <linux/buffer_head.h>
> #include <linux/magic.h>
> +#include "../nls/nls_ucs2_utils.h"
>
> #include "exfat_raw.h"
> #include "exfat_fs.h"
> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
> super_block *sb)
> return 0;
> }
>
> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
> + struct buffer_head **out_bh,
> + struct exfat_dentry **out_dentry,
> + struct inode *root_inode)
Instead of passing root_inode, it seems more helpful to pass the
need_create condition to better understand the function's behavior.
As mentioned earlier, the root_inode can be found directly from
sb->s_root->d_inode.
> +{
> + int i, ret;
> + unsigned int type, old_clu;
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> + struct exfat_chain clu;
> + struct exfat_dentry *ep, *deleted_ep = NULL;
> + struct buffer_head *bh, *deleted_bh;
> +
> + clu.dir = sbi->root_dir;
> + clu.flags = ALLOC_FAT_CHAIN;
> +
> + while (clu.dir != EXFAT_EOF_CLUSTER) {
> + for (i = 0; i < sbi->dentries_per_clu; i++) {
> + ep = exfat_get_dentry(sb, &clu, i, &bh);
> +
> + if (!ep) {
> + ret = -EIO;
> + goto end;
> + }
> +
> + type = exfat_get_entry_type(ep);
> + if (type == TYPE_DELETED && !deleted_ep &&
root_inode)
> {
> + deleted_ep = ep;
> + deleted_bh = bh;
> + continue;
> + }
> +
> + if (type == TYPE_UNUSED) {
> + if (!root_inode) {
> + brelse(bh);
> + ret = -ENOENT;
> + goto end;
> + }
Too many unnecessary operations are being performed here.
1. Since the VOLUME_LABEL entry requires only one empty entry, if a DELETED
or UNUSED entry is found, it can be used directly.
2. According to the exFAT specification, all entries after UNUSED are
guaranteed to be UNUSED.
Therefore, there is no need to allocate additional clusters or
mark the next entry as UNUSED here.
In the case of need_create(as of now, root_inode is not null),
if the next cluster is EOF and TYPE_VOLUME, TYPE_DELETED, or TYPE_UNUSED
entries are not found, then a new cluster should be allocated.
Lastly, if a new VOLUME_LABEL entry is created, initialization of
hint_femp is required.
> +
> + if (deleted_ep) {
> + brelse(bh);
> + goto end;
> + }
> +
> + if (i < sbi->dentries_per_clu - 1) {
> + deleted_ep = ep;
> + deleted_bh = bh;
> +
> + ep = exfat_get_dentry(sb, &clu,
> + i + 1, &bh);
> + memset(ep, 0,
> + sizeof(struct exfat_dentry));
> + ep->type = EXFAT_UNUSED;
> + exfat_update_bh(bh, true);
> + brelse(bh);
> +
> + goto end;
> + }
> +
> + // Last dentry in cluster
> + clu.size = 0;
> + old_clu = clu.dir;
> + ret = exfat_alloc_cluster(root_inode, 1,
> + &clu, true);
> + if (ret < 0) {
> + brelse(bh);
> + goto end;
> + }
> +
> + ret = exfat_ent_set(sb, old_clu, clu.dir);
> + if (ret < 0) {
> + exfat_free_cluster(root_inode,
&clu);
> + brelse(bh);
> + goto end;
> + }
> +
> + ret = exfat_zeroed_cluster(root_inode,
clu.dir);
> + if (ret < 0) {
> + exfat_free_cluster(root_inode,
&clu);
> + brelse(bh);
> + goto end;
> + }
> +
> + deleted_ep = ep;
> + deleted_bh = bh;
> + goto end;
> + }
> +
> + if (type == TYPE_VOLUME) {
> + *out_bh = bh;
> + *out_dentry = ep;
> +
> + if (deleted_ep)
> + brelse(deleted_bh);
> +
> + return 0;
> + }
> +
> + brelse(bh);
> + }
> +
> + if (exfat_get_next_cluster(sb, &(clu.dir))) {
> + ret = -EIO;
> + goto end;
> + }
> + }
> +
> + ret = -EIO;
> +
> +end:
> + if (deleted_ep) {
> + *out_bh = deleted_bh;
> + *out_dentry = deleted_ep;
> + memset((*out_dentry), 0, sizeof(struct exfat_dentry));
> + (*out_dentry)->type = EXFAT_VOLUME;
> + return 0;
> + }
> +
> + *out_bh = NULL;
> + *out_dentry = NULL;
> + return ret;
> +}
> +
> +static int exfat_alloc_volume_label(struct super_block *sb)
> +{
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +
> + if (sbi->volume_label)
> + return 0;
> +
> +
> + mutex_lock(&sbi->s_lock);
> + sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN,
> + sizeof(short),
GFP_KERNEL);
> + mutex_unlock(&sbi->s_lock);
> +
> + if (!sbi->volume_label)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +int exfat_read_volume_label(struct super_block *sb)
> +{
> + int ret, i;
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> + struct buffer_head *bh = NULL;
> + struct exfat_dentry *ep = NULL;
> +
> + ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL);
> + // ENOENT signifies that a volume label dentry doesn't exist
> + // We will treat this as an empty volume label and not fail.
> + if (ret < 0 && ret != -ENOENT)
> + goto cleanup;
> +
> + ret = exfat_alloc_volume_label(sb);
> + if (ret < 0)
> + goto cleanup;
> +
> + mutex_lock(&sbi->s_lock);
The sbi->s_lock should be acquired from the beginning of the function.
> + if (!ep)
> + memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN);
If sbi->volume_label remains, a memset operation is required for
EXFAT_VOLUME_LABEL_LEN * sizeof(short).
> + else
> + for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
> + sbi->volume_label[i] = le16_to_cpu(ep-
> >dentry.volume_label.volume_label[i]);
> + mutex_unlock(&sbi->s_lock);
> +
> + ret = 0;
> +
> +cleanup:
> + if (bh)
> + brelse(bh);
> +
> + return ret;
> +}
> +
> +int exfat_write_volume_label(struct super_block *sb,
> + struct exfat_uni_name *uniname,
> + struct inode *root_inode)
> +{
> + int ret, i;
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> + struct buffer_head *bh = NULL;
> + struct exfat_dentry *ep = NULL;
> +
> + if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
> + ret = -EINVAL;
> + goto cleanup;
> + }
> +
> + ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode);
> + if (ret < 0)
> + goto cleanup;
> +
> + ret = exfat_alloc_volume_label(sb);
> + if (ret < 0)
> + goto cleanup;
> +
> + memcpy(sbi->volume_label, uniname->name,
> + uniname->name_len * sizeof(short));
> +
> + mutex_lock(&sbi->s_lock);
The sbi->s_lock should be acquired from the beginning of the function.
> + for (i = 0; i < uniname->name_len; i++)
> + ep->dentry.volume_label.volume_label[i] =
> + cpu_to_le16(sbi->volume_label[i]);
> + // Fill the rest of the str with 0x0000
> + for (; i < EXFAT_VOLUME_LABEL_LEN; i++)
> + ep->dentry.volume_label.volume_label[i] = 0x0000;
> +
> + ep->dentry.volume_label.char_count = uniname->name_len;
> + mutex_unlock(&sbi->s_lock);
> +
> + ret = 0;
> +
> +cleanup:
> + if (bh) {
> + exfat_update_bh(bh, true);
> + brelse(bh);
> + }
> +
> + return ret;
> +}
> +
> /* mount the file system volume */
> static int __exfat_fill_super(struct super_block *sb,
> struct exfat_chain *root_clu)
> @@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p)
>
> unload_nls(sbi->nls_io);
> exfat_free_upcase_table(sbi);
> + kfree(sbi->volume_label);
> exfat_free_sbi(sbi);
> }
>
> --
> 2.34.1
Powered by blists - more mailing lists