[RFC,v7,1/2] Btrfs: Add a new ioctl to get the label of a mounted file system

Commit Message

jeff.liu Dec. 20, 2012, 8:43 a.m. UTC

With the new ioctl(2) BTRFS_IOC_GET_FSLABEL we can fetch the label of a mounted file system.

Signed-off-by: Jie Liu <[e-mail]>
Signed-off-by: Anand Jain <[e-mail]>
Cc: Miao Xie <[e-mail]>
Cc: Goffredo Baroncelli <[e-mail]>
Cc: David Sterba <[e-mail]>
---
 fs/btrfs/ioctl.c |   15 +++++++++++++++
 fs/btrfs/ioctl.h |    2 ++
 2 files changed, 17 insertions(+)

Comments

Goffredo Baroncelli Dec. 20, 2012, 8:18 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Anand Jain <[e-mail]>
> Cc: Miao Xie <[e-mail]>
> Cc: Goffredo Baroncelli <[e-mail]>
> Cc: David Sterba <[e-mail]>
[...]
> +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[].

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().


> +	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

jeff.liu Dec. 21, 2012, 6:42 a.m. UTC | #2

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 <[e-mail]>
>> Signed-off-by: Anand Jain <[e-mail]>
>> Cc: Miao Xie <[e-mail]>
>> Cc: Goffredo Baroncelli <[e-mail]>
>> Cc: David Sterba <[e-mail]>
> [...]
>> +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.

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stefan Behrens Dec. 21, 2012, 8:50 a.m. UTC | #3

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goffredo Baroncelli Dec. 21, 2012, 5:36 p.m. UTC | #4

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 <[e-mail]>
>>> Signed-off-by: Anand Jain <[e-mail]>
>>> Cc: Miao Xie <[e-mail]>
>>> Cc: Goffredo Baroncelli <[e-mail]>
>>> Cc: David Sterba <[e-mail]>
>> [...]
>>> +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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

jeff.liu Dec. 24, 2012, 8:07 a.m. UTC | #5

On 12/22/2012 01:36 AM, Goffredo Baroncelli wrote:
> 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 <[e-mail]>
>>>> Signed-off-by: Anand Jain <[e-mail]>
>>>> Cc: Miao Xie <[e-mail]>
>>>> Cc: Goffredo Baroncelli <[e-mail]>
>>>> Cc: David Sterba <[e-mail]>
>>> [...]
>>>> +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)
That sounds ok to me, but it's better to limit the length to be
'BTRFS_LABEL_SIZE - 1' bytes, or else, it would copy 256 bytes back to
the user space if the label length is beyond or equal to the array limits.
> 
> 
>> 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).
Maybe like following?

size_t len = strlen(label);

if (len > BTRFS_LABEL_SIZE - 1) {
	WARN(1, "btrfs: device label has weird length %zu bytes, "
		     "it will be truncated to %d bytes.\n",
		     len, BTRFS_LABEL_SIZE - 1);

	len = BTRFS_LABEL_SIZE - 1;
}

I'll post a new patch with those changes if no other objections.

Happy Christmas.
-Jeff
> 
>>
>> 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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goffredo Baroncelli Dec. 24, 2012, 1:46 p.m. UTC | #6

Hi Jeff,

On 12/24/2012 09:07 AM, Jeff Liu wrote:
> On 12/22/2012 01:36 AM, Goffredo Baroncelli wrote:
>> On 12/21/2012 07:42 AM, Jeff Liu wrote:
[...]
>>> 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)
> That sounds ok to me, but it's better to limit the length to be
> 'BTRFS_LABEL_SIZE - 1' bytes, or else, it would copy 256 bytes back to
> the user space if the label length is beyond or equal to the array limits.

Sorry I don't understand your reply. The ioctl has as argument an array
of BTRFS_LABEL_SIZE characters, so it should not be any problem of page
fault (with the exception of an  user who passes a shorter array). So it
wouldn't be any problem to copy
BTRFS_LABEL_SIZE character, or I am missing something ?

The question is another. Is a kernel responsibility to assure that the
returned string is zero terminated ? If yes (and I think so)  we should
truncate the string before the copy:

	label[BTRFS_LABEL_SIZE-1] = 0;
	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).
> Maybe like following?
> 
> size_t len = strlen(label);

If label (who is an array read from the disk) is not zero terminated,
this would lead to a possible page fault. I suggest to use strnlen() or
similar.

> 
> if (len > BTRFS_LABEL_SIZE - 1) {
> 	WARN(1, "btrfs: device label has weird length %zu bytes, "
> 		     "it will be truncated to %d bytes.\n",
> 		     len, BTRFS_LABEL_SIZE - 1);
> 
> 	len = BTRFS_LABEL_SIZE - 1;
> }

As message I suggest:

WARN(1, "btrfs: device label is not zero terminated, it will be
truncated to %d bytes.\n",
			BTRFS_LABEL-1 );			


NOTE: it is suggested to not span the string on more lines, to help
searching in the source a messages with grep. It is an exception of the
rule of the max 80 columns text width.

Happy Christmas you too
GB

> 
> I'll post a new patch with those changes if no other objections.
> 
> Happy Christmas.
> -Jeff
>>
>>>
>>> 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 [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
> 
>

jeff.liu Dec. 24, 2012, 3:10 p.m. UTC | #7

On 12/24/2012 09:46 PM, Goffredo Baroncelli wrote:
> Hi Jeff,
> 
> On 12/24/2012 09:07 AM, Jeff Liu wrote:
>> On 12/22/2012 01:36 AM, Goffredo Baroncelli wrote:
>>> On 12/21/2012 07:42 AM, Jeff Liu wrote:
> [...]
>>>> 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)
>> That sounds ok to me, but it's better to limit the length to be
>> 'BTRFS_LABEL_SIZE - 1' bytes, or else, it would copy 256 bytes back to
>> the user space if the label length is beyond or equal to the array limits.
> 
> Sorry I don't understand your reply. The ioctl has as argument an array
> of BTRFS_LABEL_SIZE characters, so it should not be any problem of page
> fault (with the exception of an  user who passes a shorter array). So it
> wouldn't be any problem to copy
> BTRFS_LABEL_SIZE character, or I am missing something ?
We can copy label length up to BTRFS_LABEL_SIZE back to the user space, 
but please see my response which were shown as following.
> 
> The question is another. Is a kernel responsibility to assure that the
> returned string is zero terminated ? If yes (and I think so)  we should
> truncate the string before the copy:
On both the old btrfslabel.c->change_label_unmounted() and the new ioctl(BTRFS_IOC_SET_FSLABE),
we all made the terminated character to be NUL(i.e. actually, the maximum length is 255), so that it's
better to assure that the returned string to be same as well IMO. 
> 
> 	label[BTRFS_LABEL_SIZE-1] = 0;
> 	copy_to_user(arg, label, BTRFS_LABEL_SIZE);
This definitely works, please see my response below again.
> 
>>>
>>>
>>>> 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).
>> Maybe like following?
>>
>> size_t len = strlen(label);
As we have already evaluated the real length of the label, why not
just use it to indicate the length that will be copied back?
> 
> If label (who is an array read from the disk) is not zero terminated,
> this would lead to a possible page fault. I suggest to use strnlen() or
> similar.
Good point, that would make code better, i.e. avoid page fault. :)
> 
>>
>> if (len > BTRFS_LABEL_SIZE - 1) {
>> 	WARN(1, "btrfs: device label has weird length %zu bytes, "
>> 		     "it will be truncated to %d bytes.\n",
>> 		     len, BTRFS_LABEL_SIZE - 1);
>>
>> 	len = BTRFS_LABEL_SIZE - 1;
>> }
> 
> As message I suggest:
> 
> WARN(1, "btrfs: device label is not zero terminated, it will be
> truncated to %d bytes.\n",
> 			BTRFS_LABEL-1 );
Ok, I'd like to follow up with your suggestions as am not english native.
> 
> 
> NOTE: it is suggested to not span the string on more lines, to help
> searching in the source a messages with grep. It is an exception of the
> rule of the max 80 columns text width.
That's why I splitted the warning info into two lines.

Thanks,
-Jeff
> 
> Happy Christmas you too
> GB
> 
>>
>> I'll post a new patch with those changes if no other objections.
>>
>> Happy Christmas.
>> -Jeff
>>>
>>>>
>>>> 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 [e-mail]
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 8fcf9a5..6a2488a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3699,6 +3699,19 @@  out:
 	return ret;
 }
 
+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));
+	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