[LTP] [PATCH] mmapstress04: rewrite to fix heap overwrite

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +/*
> + * Copyright (c) 2017 Red Hat, Inc.
> + * 01/02/2003	Port to LTP avenkat@us.ibm.com
> + * 06/30/2001	Port to Linux	nsharoff@us.ibm.com
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +/*
> + * Mmap parts of a file, and then write to the file causing single
> + * write requests to jump back and forth between mmaped io and regular io.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +
> +#define NUM_PAGES (64*8)
> +#define TEST_FILE "mmapstress04-testfile"
> +
> +static int page_size;
> +static unsigned char *mmap_area;
> +
> +static void setup(void)
> +{
> +	page_size = getpagesize();
> +
> +	/*
> +	 * Pick large enough area, PROT_NONE doesn't matter,
> +	 * because we remap it later.
> +	 */
> +	mmap_area = SAFE_MMAP(NULL, page_size * NUM_PAGES, PROT_NONE,
> +		MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> +}
> +
> +static void write_fully(int fd, void *buf, int len)
> +{
> +	do {
> +		len -= SAFE_WRITE(0, fd, buf, len);

Not that this matters here, since the buf is filled with 'a' bytes
anyway, but we should probably add a offset to the buffer on subsequent
writes or at least add a comment that we do not care. As it is the code
looks buggy.

> +	} while (len > 0);
> +}
> +
> +static void mmapstress04(void)
> +{
> +	int i, j, rofd, rwfd, ret = 0;
> +	char *buf;
> +
> +	if (tst_fill_file(TEST_FILE, 'b', page_size, 1))
> +		tst_brk(TBROK | TERRNO, "fill_file");
> +
> +	rofd = SAFE_OPEN(TEST_FILE, O_RDONLY | O_CREAT, 0777);
> +	/*
> +	 * Assuming disk blocks are 8k, and logical pages are 4k, there are
> +	 * two maps per page. In order to test mapping at the beginning and
                        ^
			block
> +	 * ends of the block, mapping the whole block, or none of the block
> +	 * with different mappings on preceding and following blocks, each
> +	 * 3 blocks with 6 pages can be thought of as a binary number from 0 to
> +	 * 64 with a bit set for mapped or cleared for unmapped. This number
> +	 * is represented by i. The value j is used to look at the bits of i
> +	 * and decided to map the page or not.
> +	 * NOTE: None of the above assumptions are critical.
> +	 */
> +	for (i = 0; i < 64; i++) {
> +		for (j = 0; j < 6; j++) {
> +			off_t mapoff;
> +
> +			if (!(i & (1 << j)))
> +				continue;
> +
> +			mapoff = page_size * (off_t)(6 + i + j);
                                                       ^
						   Should be '*'?
> +			SAFE_MMAP(mmap_area + page_size * (6 * i + j),
> +				 page_size, PROT_READ,
> +				 MAP_FILE | MAP_PRIVATE | MAP_FIXED,
> +				 rofd, mapoff);

I wonder if we can just keep a counter for the offset in the mmaped area
here, something as:

			mmapped_pages = 0;

			SAFE_MMAP(mmap_area + page_size * mapped_pages++, ...


Then the loop that checks the data could be just a simple for() loop.

> +		}
> +	}
> +	SAFE_CLOSE(rofd);
> +
> +	/* write out 6 pages of stuff into each of the 64 six page sections */
> +	rwfd = SAFE_OPEN(TEST_FILE, O_RDWR);
> +	buf = SAFE_MALLOC(page_size);
> +	memset(buf, 'a', page_size);
> +	for (i = 0; i < 6 * 64; i++)
> +		write_fully(rwfd, buf, page_size);
> +	free(buf);
> +	SAFE_CLOSE(rwfd);
> +
> +	/*
> +	 * Just finished scribbling all over interwoven mmapped and unmapped
> +	 * regions. Check the data.
> +	 */
> +	for (i = 0; i < 64; i++) {
> +		for (j = 0; j < 6; j++) {
> +			unsigned char val;
> +
> +			if (!(i & (1 << j)))
> +				continue;
> +
> +			val = *(mmap_area + page_size * (6 * i + j));
> +			if (val != 'a') {
> +				tst_res(TFAIL, "unexpected value in map, "
> +					"i=%d,j=%d,val=0x%x", i, j, val);
> +				ret = 1;
> +				goto done;
> +			}

Maybe we should check whole page, not just first byte.

> +		}
> +	}
> +done:
> +	tst_res(ret ? TFAIL : TPASS, "blocks have expected data");

This would produce failing message with 'block have expected data' text
if we get to the goto above. Why isn't the done: label pointing just
before the SAFE_UNLIK()? It does to make any sense to print the failure
message twice here.

> +	SAFE_UNLINK(TEST_FILE);
> +}
> +
> +static struct tst_test test = {
> +	.tid = "mmapstress04",
> +	.needs_tmpdir = 1,
> +	.test_all = mmapstress04,
> +	.setup = setup,
> +};
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz