diff mbox series

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

Message ID 1649763226-2329-3-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap | expand

Commit Message

Yang Xu (Fujitsu) April 12, 2022, 11:33 a.m. UTC 
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(+)

Comments

Christian Brauner April 13, 2022, 8:07 a.m. UTC | #1 
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.
Yang Xu (Fujitsu) April 13, 2022, 8:48 a.m. UTC | #2 
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
diff mbox series

Patch

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");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (!is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(t_dir1_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8018,6 +8061,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");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(t_dir1_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8039,6 +8101,9 @@  static int setgid_create_idmapped(void)
 		.attr_set = MOUNT_ATTR_IDMAP,
 	};
 	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
 
 	if (!caps_supported())
 		return 0;
@@ -8086,6 +8151,8 @@  static int setgid_create_idmapped(void)
 		goto out;
 	}
 
+	supported = openat_tmpfile_supported(open_tree_fd);
+
 	pid = fork();
 	if (pid < 0) {
 		log_stderr("failure: fork");
@@ -8168,6 +8235,25 @@  static int setgid_create_idmapped(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_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, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 10000, 10000))
+				die("failure: check ownership");
+			if  (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8191,6 +8277,9 @@  static int setgid_create_idmapped_in_userns(void)
 		.attr_set = MOUNT_ATTR_IDMAP,
 	};
 	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
 
 	if (!caps_supported())
 		return 0;
@@ -8238,6 +8327,8 @@  static int setgid_create_idmapped_in_userns(void)
 		goto out;
 	}
 
+	supported = openat_tmpfile_supported(open_tree_fd);
+
 	pid = fork();
 	if (pid < 0) {
 		log_stderr("failure: fork");
@@ -8304,6 +8395,25 @@  static int setgid_create_idmapped_in_userns(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_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, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (!is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8412,6 +8522,25 @@  static int setgid_create_idmapped_in_userns(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_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, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 0, 1000))
+				die("failure: check ownership");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8508,6 +8637,25 @@  static int setgid_create_idmapped_in_userns(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_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, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))