On 12/21/2012 07:42, Jeff Liu wrote:
On 12/21/2012 04:18 AM, Goffredo Baroncelli wrote:
On 12/20/2012 09:43 AM, Jeff Liu wrote:
+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 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.
Add BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1) is reasonable instead.
Don't allow users to attack the kernel! This would add a severe security
issue. A BUG_ON() is something that you can use before the code would
crash anyway, to prevent any additional damage and to help in debugging.
A BUG() is not a method to report or handle user errors.
A Linux system is supposed to run until it is shutdown by the
administrator, not until somebody inserts an USB stick.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
