Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755610AbdLTPtU (ORCPT ); Wed, 20 Dec 2017 10:49:20 -0500 Received: from mail.parknet.co.jp ([210.171.160.6]:34004 "EHLO mail.parknet.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755584AbdLTPtQ (ORCPT ); Wed, 20 Dec 2017 10:49:16 -0500 From: OGAWA Hirofumi To: Chen Guanqiao Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] fs: fat: add ioctl to modify fat filesystem volume label References: <20171220140547.5173-1-chen.chenchacha@foxmail.com> Date: Thu, 21 Dec 2017 00:49:13 +0900 In-Reply-To: <20171220140547.5173-1-chen.chenchacha@foxmail.com> (Chen Guanqiao's message of "Wed, 20 Dec 2017 22:05:47 +0800") Message-ID: <87k1xhh01y.fsf@mail.parknet.co.jp> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2781 Lines: 109 Chen Guanqiao 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