Re: [PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

on 2022/3/31 20:10, Christian Brauner wrote:
> On Thu, Mar 31, 2022 at 01:59:25PM +0200, Christian Brauner wrote:
>> On Thu, Mar 31, 2022 at 05:28:21PM +0800, Yang Xu wrote:
>>> Since mknodat can create file, we should also check whether strip S_ISGID.
>>> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
>>> depond on this cap and keep other cap(ie CAP_MKNOD) because create character device
>>> needs it when using mknod.
>>>
>>> Only test mknod with character device in setgid_create function because the another
>>> two functions will hit EPERM error.
>>
>> Fwiw, it's not allowed to create devices in userns as that would be a
>> massive attack vector. But it is possible since 5.<some version>  to
>> create whiteouts in userns for the sake of overlayfs. So iirc that
>> creating a whiteout is just passing 0 as dev_t:
>>
>> mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, 0)
>>
>> but you'd need to detect whether the kernel allows this and skip the
>> test on EPERM when it is a userns test.
>
> Oh, iirc Eryu usually prefers if we don't just extend existing tests but
> add new tests so as not to introduce regressions. So instead of adding
> this into the existings tests you _could_ add them as new separate
>
> struct t_idmapped_mounts t_setgid[] = {
> };
>
> set of tests and add a new command line switch:
>
> --test-setgid
>
> and create a new
>
> generic/67*
>
> for it. You can use:
> d17a88e90956 ("generic: test idmapped mount circular mappings")
> as a template for what I mean.
When I write this patchset, I also think about it. I plan to move setgid 
test from of test-core group and use a new test-segid group(also 
increase its coverage).

Will do it on v2.

Best Regards
Yang Xu