[Devel,rh7] ext4: fix filtering trusted xattr

Submitted by Maxim Patlasov on Sept. 20, 2016, 10:56 p.m.

Details

Message ID 147441217934.9189.12804393377239472465.stgit@maxim-thinkpad
State New
Series "ext4: fix filtering trusted xattr"
Headers show

Commit Message

Maxim Patlasov Sept. 20, 2016, 10:56 p.m.
Commit 4f7ce4dd4741cb65df018028aaefedb298915aa6:

Author: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
ve/xattr: allow to set trusted.xxx for container admin

relaxed capability check on setxattr path, but overlooked
to do the same on getxattr path. Hence, container admin
became able to set trusted xattrs, but not seeing them:

# setfattr -h -n trusted.name file
# echo $?
0
# getfattr -dm- file
<empty-output>

This broke generic/062 from xfstests.

https://jira.sw.ru/browse/PSBM-51009

Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
---
 fs/ext4/xattr_trusted.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/fs/ext4/xattr_trusted.c b/fs/ext4/xattr_trusted.c
index 95f1f4a..49dd83f 100644
--- a/fs/ext4/xattr_trusted.c
+++ b/fs/ext4/xattr_trusted.c
@@ -19,7 +19,7 @@  ext4_xattr_trusted_list(struct dentry *dentry, char *list, size_t list_size,
 	const size_t prefix_len = XATTR_TRUSTED_PREFIX_LEN;
 	const size_t total_len = prefix_len + name_len + 1;
 
-	if (!capable(CAP_SYS_ADMIN))
+	if (!ve_capable(CAP_SYS_ADMIN))
 		return 0;
 
 	if (list && total_len <= list_size) {

Comments

Pavel Tikhomirov Sept. 21, 2016, 6:56 a.m.
We have same check in ext2_xattr_trusted_list, ext3_xattr_trusted_list, 
hfsplus_listxattr and more, all together ~15 places. Do we need list 
only on ext4? Maybe we can be good with only getxattr allowed without list?

On 09/21/2016 01:56 AM, Maxim Patlasov wrote:
> Commit 4f7ce4dd4741cb65df018028aaefedb298915aa6:
>
> Author: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ve/xattr: allow to set trusted.xxx for container admin
>
> relaxed capability check on setxattr path, but overlooked
> to do the same on getxattr path. Hence, container admin
> became able to set trusted xattrs, but not seeing them:
>
> # setfattr -h -n trusted.name file
> # echo $?
> 0
> # getfattr -dm- file
> <empty-output>
>
> This broke generic/062 from xfstests.
>
> https://jira.sw.ru/browse/PSBM-51009
>
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
> ---
>  fs/ext4/xattr_trusted.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/xattr_trusted.c b/fs/ext4/xattr_trusted.c
> index 95f1f4a..49dd83f 100644
> --- a/fs/ext4/xattr_trusted.c
> +++ b/fs/ext4/xattr_trusted.c
> @@ -19,7 +19,7 @@ ext4_xattr_trusted_list(struct dentry *dentry, char *list, size_t list_size,
>  	const size_t prefix_len = XATTR_TRUSTED_PREFIX_LEN;
>  	const size_t total_len = prefix_len + name_len + 1;
>
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!ve_capable(CAP_SYS_ADMIN))
>  		return 0;
>
>  	if (list && total_len <= list_size) {
>
Konstantin Khorenko Sept. 21, 2016, 1:46 p.m.
On 09/21/2016 09:56 AM, Pavel Tikhomirov wrote:
> We have same check in ext2_xattr_trusted_list, ext3_xattr_trusted_list,
> hfsplus_listxattr and more, all together ~15 places. Do we need list
> only on ext4? Maybe we can be good with only getxattr allowed without list?

Let's behave the same as in a VM.

"trusted.xxx" are listed in VMs => let's list them inside Containers.

About fixing this for ext4 only: well, yes, better to fix it for other fs as well,
but i think we can skip it: we allow only ext4 inside ploop =>
we'll be fine.
If some days someone comes to us with other fs, we'll fix it as well.
(People can face it if run on "simfs" (which is a bindmount now) over, say, zfs or xfs).

>
> On 09/21/2016 01:56 AM, Maxim Patlasov wrote:
>> Commit 4f7ce4dd4741cb65df018028aaefedb298915aa6:
>>
>> Author: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ve/xattr: allow to set trusted.xxx for container admin
>>
>> relaxed capability check on setxattr path, but overlooked
>> to do the same on getxattr path. Hence, container admin
>> became able to set trusted xattrs, but not seeing them:
>>
>> # setfattr -h -n trusted.name file
>> # echo $?
>> 0
>> # getfattr -dm- file
>> <empty-output>
>>
>> This broke generic/062 from xfstests.
>>
>> https://jira.sw.ru/browse/PSBM-51009
>>
>> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
>> ---
>>  fs/ext4/xattr_trusted.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/xattr_trusted.c b/fs/ext4/xattr_trusted.c
>> index 95f1f4a..49dd83f 100644
>> --- a/fs/ext4/xattr_trusted.c
>> +++ b/fs/ext4/xattr_trusted.c
>> @@ -19,7 +19,7 @@ ext4_xattr_trusted_list(struct dentry *dentry, char *list, size_t list_size,
>>  	const size_t prefix_len = XATTR_TRUSTED_PREFIX_LEN;
>>  	const size_t total_len = prefix_len + name_len + 1;
>>
>> -	if (!capable(CAP_SYS_ADMIN))
>> +	if (!ve_capable(CAP_SYS_ADMIN))
>>  		return 0;
>>
>>  	if (list && total_len <= list_size) {
>>
>