| 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: <CAKYAXd8xTwY7SZb9OjiTF4yA127TjZMawzZRcFW7uZt8d3PPfw@mail.gmail.com>
Date: Tue, 19 Aug 2025 23:51:18 +0900
From: Namjae Jeon <linkinjeon@...nel.org>
To: Ethan Ferguson <ethan.ferguson@...ier.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
sj1557.seo@...sung.com, yuezhang.mo@...y.com
Subject: Re: [PATCH] exfat: Add support for FS_IOC_{GET,SET}FSLABEL
On Tue, Aug 19, 2025 at 10:22 PM Ethan Ferguson
<ethan.ferguson@...ier.com> wrote:
>
> On 8/18/25 21:45, Namjae Jeon wrote:
> > On Sun, Aug 17, 2025 at 9:31 AM Ethan Ferguson
> > <ethan.ferguson@...ier.com> wrote:
> >>
> >> 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 | 2 +
> >> fs/exfat/exfat_raw.h | 6 +++
> >> fs/exfat/file.c | 56 +++++++++++++++++++++++++
> >> fs/exfat/super.c | 99 ++++++++++++++++++++++++++++++++++++++++++++
> >> 4 files changed, 163 insertions(+)
> >>
> >> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> >> index f8ead4d47ef0..a764e6362172 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[EXFAT_VOLUME_LABEL_LEN]; /* volume name */
> > There's no reason to have this in sbi. I think it's better to read the
> > volume name in ioctl fslabel and return it.
> >
> That's fair. I wrote it this way because the volume label is stored in
> the sbi in btrfs, but there it's (as far as I understand) part of the
> fs header on disk, and not (as is the case in exfat) a directory entry
> that could be arbitrarily far from the start of the disk. Maybe we could
> cache it in the sbi after the first read? I'm open to either.
I agree to cache it in sbi after the first read.
>
> >>
> >> unsigned int clu_srch_ptr; /* cluster search pointer */
> >> unsigned int used_clusters; /* number of used clusters */
> >> @@ -431,6 +432,7 @@ static inline loff_t exfat_ondisk_size(const struct inode *inode)
> >> /* super.c */
> >> int exfat_set_volume_dirty(struct super_block *sb);
> >> int exfat_clear_volume_dirty(struct super_block *sb);
> >> +int exfat_write_volume_label(struct super_block *sb);
> >>
> >> /* fatent.c */
> >> #define exfat_get_next_cluster(sb, pclu) exfat_ent_get(sb, *(pclu), pclu)
> >> diff --git a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h
> >> index 971a1ccd0e89..af04cef81c0c 100644
> >> --- a/fs/exfat/exfat_raw.h
> >> +++ b/fs/exfat/exfat_raw.h
> >> @@ -80,6 +80,7 @@
> >> #define BOOTSEC_OLDBPB_LEN 53
> >>
> >> #define EXFAT_FILE_NAME_LEN 15
> >> +#define EXFAT_VOLUME_LABEL_LEN 11
> >>
> >> #define EXFAT_MIN_SECT_SIZE_BITS 9
> >> #define EXFAT_MAX_SECT_SIZE_BITS 12
> >> @@ -159,6 +160,11 @@ struct exfat_dentry {
> >> __le32 start_clu;
> >> __le64 size;
> >> } __packed upcase; /* up-case table directory entry */
> >> + struct {
> >> + __u8 char_count;
> >> + __le16 volume_label[EXFAT_VOLUME_LABEL_LEN];
> >> + __u8 reserved[8];
> >> + } __packed volume_label;
> >> struct {
> >> __u8 flags;
> >> __u8 vendor_guid[16];
> >> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> >> index 538d2b6ac2ec..c57d266aae3d 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,6 +487,57 @@ 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_sb_info *sbi = EXFAT_SB(sb);
> >> + size_t len = UniStrnlen(sbi->volume_label, EXFAT_VOLUME_LABEL_LEN);
> >> +
> >> + mutex_lock(&sbi->s_lock);
> >> + ret = utf16s_to_utf8s(sbi->volume_label, len,
> >> + UTF16_HOST_ENDIAN, utf8, FSLABEL_MAX);
> >> + mutex_unlock(&sbi->s_lock);
> >> +
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX))
> >> + return -EFAULT;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int exfat_ioctl_set_volume_label(struct super_block *sb, unsigned long arg)
> >> +{
> >> + int ret = 0;
> >> + char utf8[FSLABEL_MAX];
> >> + size_t len;
> >> + unsigned short utf16[EXFAT_VOLUME_LABEL_LEN] = {0};
> >> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> >> +
> >> + if (!capable(CAP_SYS_ADMIN))
> >> + return -EPERM;
> >> +
> >> + if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX))
> >> + return -EFAULT;
> >> +
> >> + len = strnlen(utf8, FSLABEL_MAX);
> >> + if (len > EXFAT_VOLUME_LABEL_LEN)
> > Is FSLABEL_MAX in bytes or the number of characters ?
> >
> the definition mentions chars, and everywhere else it's used it's in
> terms of chars, so I'd say it's in terms of bytes. The
> FS_IOC_{GET,SET}FSLABEL ioctls are in terms of char[FSLABEL_MAX], so
> I think it's reasonable to use it as a number of bytes.
Okay.
>
> >> + exfat_info(sb, "Volume label length too long, truncating");
> >> +
> >> + mutex_lock(&sbi->s_lock);
> >> + ret = utf8s_to_utf16s(utf8, len, UTF16_HOST_ENDIAN, utf16, EXFAT_VOLUME_LABEL_LEN);
> >> + mutex_unlock(&sbi->s_lock);
> >> +
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + memcpy(sbi->volume_label, utf16, sizeof(sbi->volume_label));
> >> +
> >> + return exfat_write_volume_label(sb);
> >> +}
> >> +
> >> long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >> {
> >> struct inode *inode = file_inode(filp);
> >> @@ -500,6 +552,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);
> >> default:
> >> return -ENOTTY;
> >> }
> >> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> >> index 8926e63f5bb7..96cd4bb7cb19 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,98 @@ 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)
> >> +{
> >> + int i;
> >> + unsigned int type;
> >> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> >> + struct exfat_chain clu;
> >> + struct exfat_dentry *ep;
> >> + struct buffer_head *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)
> >> + return -EIO;
> >> +
> >> + type = exfat_get_entry_type(ep);
> >> + if (type == TYPE_UNUSED) {
> >> + brelse(bh);
> >> + return -EIO;
> >> + }
> >> +
> >> + if (type == TYPE_VOLUME) {
> >> + *out_bh = bh;
> >> + *out_dentry = ep;
> >> + return 0;
> >> + }
> >> +
> >> + brelse(bh);
> >> + }
> >> +
> >> + if (exfat_get_next_cluster(sb, &(clu.dir)))
> >> + return -EIO;
> >> + }
> >> +
> >> + return -EIO;
> >> +}
> >> +
> >> +static int exfat_read_volume_label(struct super_block *sb)
> >> +{
> >> + int ret, i;
> >> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> >> + struct buffer_head *bh;
> >> + struct exfat_dentry *ep;
> >> +
> >> + ret = exfat_get_volume_label_ptrs(sb, &bh, &ep);
> >> + if (ret < 0)
> >> + goto cleanup;
> >> +
> >> + for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
> >> + sbi->volume_label[i] = le16_to_cpu(ep->dentry.volume_label.volume_label[i]);
> >> +
> >> +cleanup:
> >> + if (bh)
> >> + brelse(bh);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +int exfat_write_volume_label(struct super_block *sb)
> >> +{
> >> + int ret, i;
> >> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> >> + struct buffer_head *bh;
> >> + struct exfat_dentry *ep;
> >> +
> >> + ret = exfat_get_volume_label_ptrs(sb, &bh, &ep);
> >> + if (ret < 0)
> >> + goto cleanup;
> >> +
> >> + mutex_lock(&sbi->s_lock);
> >> + for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
> >> + ep->dentry.volume_label.volume_label[i] = cpu_to_le16(sbi->volume_label[i]);
> >> +
> >> + ep->dentry.volume_label.char_count =
> >> + UniStrnlen(sbi->volume_label, EXFAT_VOLUME_LABEL_LEN);
> >> + mutex_unlock(&sbi->s_lock);
> >> +
> >> +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)
> >> @@ -616,6 +709,12 @@ static int __exfat_fill_super(struct super_block *sb,
> >> goto free_bh;
> >> }
> >>
> >> + ret = exfat_read_volume_label(sb);
> > It will affect mount time if volume label entry is located at the end.
> > So, we can read it in ioctl fslabel as I said above.
> Sounds good. I'll incorporate your changes, and those of
> yuezhang.mo@...y.com, and submit version 3 of the patch soon.
As Yuezhang pointed out, You need to consider the case where there
is no volume label entry. Some mkfs implementations, though not
Windows, don't create a volume entry. So, if FS_IOC_SETFSLABEL
doesn't find a volume label entry, it should either look for an empty entry,
or, if that's not available, allocate a cluster to get a entry.
Thanks for your work!
>
> >> + if (ret) {
> >> + exfat_err(sb, "failed to read volume label");
> >> + goto free_bh;
> >> + }
> >> +
> >> ret = exfat_count_used_clusters(sb, &sbi->used_clusters);
> >> if (ret) {
> >> exfat_err(sb, "failed to scan clusters");
> >> --
> >> 2.50.1
> >>
Powered by blists - more mailing lists