Re: [LTP] [PATCH v1] security/dirtyc0w_shmem: Add new test for CVE-2022-2590

[David Hildenbrand <david@redhat.com>]

On 18.11.22 14:29, Cyril Hrubis wrote:
> Hi!

Hi Cyril,

thanks for your review!

[...]

>> +
>> +/*
>> + * CVE-2022-2590
>> + *
>> + * This is a regression test for a write race that allows unprivileged programs
>> + * to change readonly files located on tmpfs/shmem on the system.
>> + *
>> + * Fixed by:
>> + *
>> + *   commit 5535be3099717646781ce1540cf725965d680e7b
>> + *   Author: David Hildenbrand <david@redhat.com>
>> + *   Date:   Tue Aug 9 22:56:40 2022 +0200
>> + *
>> + *   mm/gup: fix FOLL_FORCE COW security issue and remove FOLL_COW
>> + */
> 
> This is not a proper documentation comment. We do have a system in place
> that picks up ascii-doc formatted documentation comments and builds a
> documentation based on that.
> 
> The documentation comment has to start with:
> 
> /*\
>   * [Description]
>   *
> 
> 
> Also the CVE and kernel commit are picked from tags when documentation
> is build, so they do not need to be part of the documentation comment.
> 

I used dirtyc0w.c as orientation, which probably wasn't a good idea.

I'll use a simple

/*\
  * [Description]
  *
  * This is a regression test for a write race that allowed unprivileged programs
  * to change readonly files located on tmpfs/shmem on the system using
  * userfaultfd "minor fault handling" (CVE-2022-2590).
  */

>> +#include "config.h"
>> +
>> +#include <pthread.h>
>> +#include <unistd.h>
>> +#include <sys/stat.h>
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <stdbool.h>
>> +#include <pwd.h>
>> +
>> +#include "tst_test.h"
>> +
>> +#define TMP_DIR "tmp_dirtyc0w_shmem"
>> +#define TEST_FILE TMP_DIR"/testfile"
>> +#define STR "this is not a test\n"
>> +
>> +static uid_t nobody_uid;
>> +static gid_t nobody_gid;
>> +static bool child_early_exit;
> 
> Anything that is changed from signal handler should be volatile
> otherwise it may end up optimized out.

Ack.

> 
>> +static void sighandler(int sig)
>> +{
>> +	if (sig == SIGCHLD) {
>> +		child_early_exit = true;
>> +		return;
>> +	}
>> +
>> +	_exit(0);
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	struct passwd *pw;
>> +
>> +	umask(0);
>> +
>> +	pw = SAFE_GETPWNAM("nobody");
>> +
>> +	nobody_uid = pw->pw_uid;
>> +	nobody_gid = pw->pw_gid;
>> +
>> +	SAFE_MKDIR(TMP_DIR, 0664);
>> +	SAFE_MOUNT(TMP_DIR, TMP_DIR, "tmpfs", 0, NULL);
>> +}
>> +
>> +static void dirtyc0w_shmem_test(void)
>> +{
>> +	bool failed = false;
>> +	int fd, pid;
>> +	char c;
>> +
>> +	fd = SAFE_OPEN(TEST_FILE, O_WRONLY|O_CREAT|O_EXCL, 0444);
>> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, STR, sizeof(STR)-1);
>> +	SAFE_CLOSE(fd);
> 
> SAFE_FILE_PRINTF() ?

AFAIU, that would mean

SAFE_FILE_PRINTF(TEST_FILE, "%s", STR);
SAFE_CHMOD(TEST_FILE, 0444);

I'll give that a churn.

> 
>> +	pid = SAFE_FORK();
>> +	if (!pid) {
>> +		SAFE_SETGID(nobody_gid);
>> +		SAFE_SETUID(nobody_uid);
>> +		SAFE_EXECLP("dirtyc0w_shmem_child", "dirtyc0w_shmem_child", NULL);
>> +	}
>> +
>> +	TST_CHECKPOINT_WAIT(0);
>> +
>> +	SAFE_SIGNAL(SIGCHLD, sighandler);
>> +	do {
>> +		usleep(100000);
>> +
>> +		SAFE_FILE_SCANF(TEST_FILE, "%c", &c);
>> +
>> +		if (c != 't') {
>> +			failed = true;
>> +			break;
>> +		}
>> +	} while (tst_remaining_runtime() && !child_early_exit);
>> +	SAFE_SIGNAL(SIGCHLD, SIG_DFL);
>> +
>> +	SAFE_KILL(pid, SIGUSR1);
>> +	tst_reap_children();
>> +	SAFE_UNLINK(TEST_FILE);
>> +
>> +	if (child_early_exit)
>> +		tst_res(TINFO, "Early child process exit");
> 
> This will potentionally trigger library detection for buggy tests. A
> test has to report a result in all situations.
> 
> I suppose that this will only happen if child process did exit with
> TBROK or TCONF. If that is the case it should be fine.

Yes, exactly. The child process will only quit in case it fails with
TBROK / TCONF. In all other cases, it will simply run until we zap it.

> 
>> +	else if (failed)
>> +		tst_res(TFAIL, "Bug reproduced!");
>> +	else
>> +		tst_res(TPASS, "Bug not reproduced");
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	SAFE_UMOUNT(TMP_DIR);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.needs_checkpoints = 1,
>> +	.forks_child = 1,
>> +	.needs_root = 1,
> 
> We are missing .needs_tmpdir flag here, otherwise the test will create
> the the TMPDIR in PWD which may fail in certain setups.

Thanks!


-- 
Thanks,

David / dhildenb


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