linux-kernel - Re: [PATCH v5 2/2] fs: fat: add ioctl method in fat filesystem driver

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]

Message-ID: <87efngwb7h.fsf@mail.parknet.co.jp>
Date:   Wed, 27 Dec 2017 22:29:38 +0900
From:   OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:     ChenGuanqiao <chen.chenchacha@...mail.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] fs: fat: add ioctl method in fat filesystem driver

ChenGuanqiao <chen.chenchacha@...mail.com> writes:

> +/* the characters in this field shall be d-characters, and unused byte shall be set to 0x20. */
> +static int fat_format_d_characters(char *label, unsigned long len)
> +{
> +	int i;
> +
> +	for (i=0; i<len; ++i) {
> +		switch (label[i]) {
> +		case '0' ... '9':
> +		case 'A' ... 'Z':
> +		case '_':
> +			continue;
> +		default:
> +			break;
> +		}
> +		break;
> +	}
> +
> + 	for (; i<len; ++i)
> +		label[i] = 0x20;
> +
> +	return len;
> +}

It should not accept and overwrite invalid char, return -EINVAL.

> +static int fat_ioctl_get_volume_label(struct inode *inode,
> +				      u8 __user *vol_label)
> +{
> +	int err = 0;
> +	struct fat_slot_info sinfo;
> +
> +	err = fat_scan_volume_label(inode, &sinfo);
> +	if (err)
> +		goto out;

It needs to take inode read lock.

> +	if (copy_to_user(vol_label, sinfo.de->name, MSDOS_NAME))
> +		err = -EFAULT;
> +
> +	brelse(sinfo.bh);
> +out:
> +	return err;
> +}
> +
> +static int fat_ioctl_set_volume_label(struct file *file,
> +				      u8 __user *vol_label)
> +{

[...]

> +	b = (struct fat_boot_sector *)bh->b_data;
> +
> +	lock_buffer(bh);

Probably, sb->s_umount to exclusive with remount update.

> +	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);
> +	unlock_buffer(bh);
> +	err = sync_dirty_buffer(bh);
> +	brelse(bh);
> +	if (err)
> +		goto out_drop_file;

It should not update before preparing is done and checking error.

> +	/* updates root directory's vol_label */
> +	err = fat_scan_volume_label(inode, &sinfo);
> +	if (err)
> +		goto out_drop_file;

It should add entry if no volume label.

> +	lock_buffer(bh);

inode write lock.

> +	memcpy(sinfo.de->name, label, sizeof(sinfo.de->name));
> +	mark_buffer_dirty(bh);
> +	unlock_buffer(bh);

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