Re: [PATCH v3 3/5] idmapped-mounts: Add open with O_TMPFILE operation in setgid test

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

on 2022/4/13 16:07, Christian Brauner wrote:
> On Tue, Apr 12, 2022 at 07:33:44PM +0800, Yang Xu wrote:
>> Since we can create temp file by using O_TMPFILE flag and filesystem driver also
>> has this api, we should also check this operation whether strip S_ISGID.
>>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   src/idmapped-mounts/idmapped-mounts.c | 148 ++++++++++++++++++++++++++
>>   1 file changed, 148 insertions(+)
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index 617f56e0..02f91558 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -51,6 +51,7 @@
>>   #define FILE1_RENAME "file1_rename"
>>   #define FILE2 "file2"
>>   #define FILE2_RENAME "file2_rename"
>> +#define FILE3 "file3"
>>   #define DIR1 "dir1"
>>   #define DIR2 "dir2"
>>   #define DIR3 "dir3"
>> @@ -337,6 +338,24 @@ out:
>>   	return fret;
>>   }
>>
>> +static bool openat_tmpfile_supported(int dirfd)
>> +{
>> +	int fd = -1;
>> +
>> +	fd = openat(dirfd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
>> +	if (fd == -1) {
>> +		if (errno == ENOTSUP)
>> +			return false;
>> +		else
>> +			return log_errno(false, "failure: create");
>> +	}
>> +
>> +	if (close(fd))
>> +		log_stderr("failure: close");
>> +
>> +	return true;
>> +}
>> +
>>   /* __expected_uid_gid - check whether file is owned by the provided uid and gid */
>>   static bool __expected_uid_gid(int dfd, const char *path, int flags,
>>   			       uid_t expected_uid, gid_t expected_gid, bool log)
>> @@ -7841,7 +7860,10 @@ static int setgid_create(void)
>>   {
>>   	int fret = -1;
>>   	int file1_fd = -EBADF;
>> +	int tmpfile_fd = -EBADF;
>>   	pid_t pid;
>> +	bool supported = false;
>> +	char path[PATH_MAX];
>>
>>   	if (!caps_supported())
>>   		return 0;
>> @@ -7866,6 +7888,8 @@ static int setgid_create(void)
>>   		goto out;
>>   	}
>>
>> +	supported = openat_tmpfile_supported(t_dir1_fd);
>> +
>>   	pid = fork();
>>   	if (pid<  0) {
>>   		log_stderr("failure: fork");
>> @@ -7929,6 +7953,25 @@ static int setgid_create(void)
>>   		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
>>   			die("failure: delete");
>>
>> +		/* create tmpfile via filesystem tmpfile api */
>> +		if (supported) {
>> +			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
>> +			if (tmpfile_fd<  0)
>> +				die("failure: create");
>> +			/* link the temporary file into the filesystem, making it permanent */
>> +			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
>> +			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
>> +				die("failure: linkat");
>
> Fwiw, I don't think you need that snprintf() dance as you should be able
> to use AT_EMPTY_PATH:
>
> if (linkat(fd, "", t_dir1_fd, FILE3, AT_EMPTY_PATH))
>
> for this.
Oh, Yes, it works well. Thanks.

ps:I also use this way but failed before(I used wrong argument NULL 
instead of "" when see open(2) man-pages ) .

Best Regards
Yang Xu