On 12/21/2012 07:42 AM, Jeff Liu wrote: > Hi Goffredo, > > On 12/21/2012 04:18 AM, Goffredo Baroncelli wrote: >> HI Jeff, >> >> On 12/20/2012 09:43 AM, Jeff Liu wrote: >>> With the new ioctl(2) BTRFS_IOC_GET_FSLABEL we can fetch the label of a >>> mounted file system. >>> >>> Signed-off-by: Jie Liu <jeff....@oracle.com> >>> Signed-off-by: Anand Jain <anand.j...@oracle.com> >>> Cc: Miao Xie <mi...@cn.fujitsu.com> >>> Cc: Goffredo Baroncelli <kreij...@inwind.it> >>> Cc: David Sterba <dste...@suse.cz> >> [...] >>> +static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) >>> +{ >>> + struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root; >>> + const char *label = root->fs_info->super_copy->label; >>> + int ret; >>> + >>> + mutex_lock(&root->fs_info->volume_mutex); >>> + ret = copy_to_user(arg, label, strlen(label)); >> >> Sorry for pointing out my doubt too late, but should we trust >> super_copy->label ? >> An user could insert a usb-key with a btrfs filesystem with a label >> without zero. In this case strlen() could access outside >> super_copy->label[]. > Thank you for letting me be aware of this situation. > > First of all, if the user set label via btrfs tools, he can not make it > length exceeding BTRFS_LABLE_SIZE - 1. > > If the user does that through codes wrote by himself like: > btrfslabel.c->set_label_unmounted(), he can do that. > However, it most likely he did that for evil purpose or any other reasons?
I think the most likely case is the "evil purpose". >> >> I think that it should be quite easy to alter artificially a filesystem >> to crash the kernel. So I not consider this as big problem. However *in >> case* of a further cycle of this patch I suggest to replace strlen() >> with strnlen(). > I don't think we should replace strlen() with strnlen() since it's > totally wrong if the length of label is more than BTRFS_LABEL_SIZE -1, > we can not just truncating the label and return it in this case. This for me is sufficient, or we could copy all the label buffer, without further check: copy_to_user(arg, label, BTRFS_LABEL_SIZE) > Add BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1) is reasonable instead. I agree with Stefan, this is not a correct use of BUG_ON; a warning is sufficient (there is un-correct data read from disk). > > Thanks, > -Jeff >> >>> + mutex_unlock(&root->fs_info->volume_mutex); >>> + >>> + return ret ? -EFAULT : 0; >>> +} >>> + >>> long btrfs_ioctl(struct file *file, unsigned int >>> cmd, unsigned long arg) >>> { >>> @@ -3797,6 +3810,8 @@ long btrfs_ioctl(struct file *file, unsigned int >>> return btrfs_ioctl_qgroup_create(root, argp); >>> case BTRFS_IOC_QGROUP_LIMIT: >>> return btrfs_ioctl_qgroup_limit(root, argp); >>> + case BTRFS_IOC_GET_FSLABEL: >>> + return btrfs_ioctl_get_fslabel(file, argp); >>> } >>> >>> return -ENOTTY; >>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h >>> index 731e287..5b2cbef 100644 >>> --- a/fs/btrfs/ioctl.h >>> +++ b/fs/btrfs/ioctl.h >>> @@ -451,6 +451,8 @@ struct btrfs_ioctl_send_args { >>> struct btrfs_ioctl_qgroup_create_args) >>> #define BTRFS_IOC_QGROUP_LIMIT _IOR(BTRFS_IOCTL_MAGIC, 43, \ >>> struct btrfs_ioctl_qgroup_limit_args) >>> +#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \ >>> + char[BTRFS_LABEL_SIZE]) >>> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \ >>> struct btrfs_ioctl_get_dev_stats) >>> #endif >> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html