Re: [LTP] [PATCH v4] mount08.c: Restrict overmounting of ephemeral entities

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Signed-off-by: Wei Gao <wegao@suse.com>
> Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  runtest/syscalls                           |  1 +
>  testcases/kernel/syscalls/mount/.gitignore |  1 +
>  testcases/kernel/syscalls/mount/mount08.c  | 57 ++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/mount/mount08.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index ded035ee8..d3abc8b85 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -852,6 +852,7 @@ mount04 mount04
>  mount05 mount05
>  mount06 mount06
>  mount07 mount07
> +mount08 mount08
>  
>  mount_setattr01 mount_setattr01
>  
> diff --git a/testcases/kernel/syscalls/mount/.gitignore b/testcases/kernel/syscalls/mount/.gitignore
> index 80885dbf0..3eee5863a 100644
> --- a/testcases/kernel/syscalls/mount/.gitignore
> +++ b/testcases/kernel/syscalls/mount/.gitignore
> @@ -6,3 +6,4 @@
>  /mount05
>  /mount06
>  /mount07
> +/mount08
> diff --git a/testcases/kernel/syscalls/mount/mount08.c b/testcases/kernel/syscalls/mount/mount08.c
> new file mode 100644
> index 000000000..1938c5519
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mount/mount08.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * Verify that mount will raise ENOENT if we try to mount on magic links
> + * under /proc/<pid>/fd/<nr>.
> + */
> +
> +#include "tst_test.h"
> +#include <sys/mount.h>
> +#include "tst_safe_file_at.h"
> +
> +#define MNTPOINT "mntpoint"
> +#define FOO MNTPOINT "/foo"
> +#define BAR MNTPOINT "/bar"
> +
> +static void run(void)
> +{
> +	char path[PATH_MAX];
> +	int foo_fd, newfd, proc_fd;
> +
> +	foo_fd = SAFE_OPEN(FOO, O_RDONLY | O_NONBLOCK, 0640);
> +	newfd = SAFE_DUP(foo_fd);
> +	SAFE_CLOSE(foo_fd);

AFAIK the dup() here is not needed, the original reproducer used the
dup() to create a file descriptor with exact number for the loop. In
this case we can just take the fd from SAFE_OPEN() and use it instead of
the newfd.

> +	sprintf(path, "/proc/%d/fd/%d", getpid(), newfd);
> +
> +	proc_fd = SAFE_OPENAT(AT_FDCWD, path, O_PATH | O_NOFOLLOW);
> +
> +	sprintf(path, "/proc/%d/fd/%d", getpid(), proc_fd);
> +
> +	TST_EXP_FAIL(
> +		mount(BAR, path, "", MS_BIND, 0),
> +		ENOENT,
> +		"mount() on proc failed expectedly"
                ^
		This message is used even if the call passed
		unexpectedly, so it should describe what is being done
		rather than the expected outcome, e.g.:

		"mount(/proc/$PID/fd/$FD)"
> +	);

You are not closing the file descriptors here, so the test fails with:
"-i 1000" command line params.

> +}
> +
> +static void setup(void)
> +{
> +	SAFE_CREAT(FOO, 0777);
> +	SAFE_CREAT(BAR, 0777);

This leaks two file descriptors too, but at least it's these are not
opened on each iteration.

If you just need to create a file do SAFE_TOUCH(FOO, 0777, NULL)
instead.

> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = run,
> +	.needs_root = 1,
> +	.mntpoint = MNTPOINT,
> +	.min_kver = "6.12",

With this you are masking the problem on older kernels, please do not,
as the problem has possible security implications.

> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "d80b065bb172"},
> +		{}
> +	}
> +};
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp