linux-kernel - Re: [PATCH] fs: fat: add ioctl to modify fat filesystem volume label

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>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 21 Dec 2017 00:49:13 +0900
From:   OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:     Chen Guanqiao <chen.chenchacha@...mail.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs: fat: add ioctl to modify fat filesystem volume label

Chen Guanqiao <chen.chenchacha@...mail.com> writes:

> The FAT filesystem volume label can be read with FAT_IOCTL_GET_VOLUME_LABEL
> and written with FAT_IOCTL_SET_VOLUME_LABEL.

Those vol_label should be matching with volume label in root directory,
right? So I think handling only boot sector's vol_label would not work
as expected.

> +static int fat_ioctl_get_volume_label(struct inode *inode,
> +									  u32 __user *user_attr)

Maybe you are using non-8 tab size, and so over 80 column.

> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> +	u8 __user *vol_label = (u8 __user *)user_attr;

This should not use strange cast (u32 => u8), instead caller should cast
to proper one.

> +	return copy_to_user(vol_label, sbi->vol_label, sizeof(sbi->vol_label));
> +}

Returning result of copy_to_user() is strange. Probably, it should
return 0 or -EFAULT.

> +static int fat_ioctl_set_volume_label(struct inode *inode,
> +									  u32 __user *user_attr)

same indent issue.

> +{
> +	struct buffer_head *bh;
> +	struct fat_boot_sector *b;
> +	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> +	u8 __user *vol_label = (u8 __user *)user_attr;
> +	u8 label[11];

It should not allow to change for normal user that having only read access.

> +	if (copy_from_user(label, vol_label, sizeof(label)))
> +		return -EFAULT;

It should check invalid label early (e.g. lower case chars, invalid
chars, etc.).

> +	if (sb_rdonly(inode->i_sb))
> +		return -EFAULT;

-EROFS

> +	bh = sb_bread(inode->i_sb, 0);
> +	if (bh == NULL) {
> +		fat_msg(inode->i_sb, KERN_ERR,
> +				"unable to read boot sector to write volume label");

indent issue.

> +		return -EFAULT;
> +	}

It should take lock to prevent race.

> +	b = (struct fat_boot_sector *) bh->b_data;
> +
> +	if (sbi->fat_bits == 32)
> +		memcpy(b->fat32.vol_label, label, sizeof(label));
> +	else
> +		memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> +	mark_buffer_dirty(bh);
> +	sync_dirty_buffer(bh);

It should check I/O error.

>  long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {

	u8 __user *user_vol_label = (u8 __user *)arg;

> +	case FAT_IOCTL_GET_VOLUME_LABEL:
> +		return fat_ioctl_get_volume_label(inode, user_attr);
> +	case FAT_IOCTL_SET_VOLUME_LABEL:
> +		return fat_ioctl_set_volume_label(inode, user_attr);

s/user_attr/user_vol_label/

> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 20a0a89eaca5..9991500c98af 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -45,12 +45,14 @@ struct fat_bios_param_block {
>
> 	u8	fat16_state;
> 	u32	fat16_vol_id;
> +	u8  fat16_vol_label[11];

> +	u8  fat32_vol_label[11];

indent.

> +#define FAT_IOCTL_SET_VOLUME_LABEL	_IOR('r', 0x15, __u8[11])

_IOW

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>