linux-kernel - Re: [PATCH 3/3] x86, mpx, selftests: add MPX self test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160608132944.GA18019@gmail.com>
Date:	Wed, 8 Jun 2016 15:29:44 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Dave Hansen <dave@...1.net>
Cc:	linux-kernel@...r.kernel.org, x86@...nel.org,
	dave.hansen@...ux.intel.com
Subject: Re: [PATCH 3/3] x86, mpx, selftests: add MPX self test


These patches look good to me, but it would be nice to get rid of uglies like:

> +/*
> + * Written by Dave Hansen <dave.hansen@...el.com>
> + *
> + * run like this:
> + pid=31390; BDIR="$(cat /proc/$pid/smaps | grep -B1 2097152 | head -1 | awk -F- '{print $1}')"; ./mpx-dig $pid 0x$BDIR
> +
> +NOTE:
> + assumes that the only 2097152-kb VMA is the bounds dir
> + */

(weird vertical alignment)

> +long nr_incore(void *ptr, unsigned long size_bytes)
> +{
> +	int i;
> +	long ret = 0;
> +	long vec_len = size_bytes / PAGE_SIZE;
> +	unsigned char *vec = malloc(vec_len);
> +	if (!vec)

(missing newline)

> +		mpx_dig_abort();
> +
> +	int incore_ret = mincore(ptr, size_bytes, vec);

(weird non-kernel-style definition.)

> +char buf[100];
> +int open_proc(int pid, char *file)

(missing newline)

> +{
> +	int fd;
> +	sprintf(&buf[0], "/proc/%d/%s", pid, file);

(ditto, also unsafe looking sprintf())

> +void __dave_abort(int line)
> +{
> +	perror("abort");
> +	printf("abort @ %d\n", line);
> +	mpx_dig_abort();
> +}
> +#define dave_mpx_dig_abort() __dave_abort(__LINE__);

maybe s/dave// ? ;-)

> +//This mincore stuff works, but the bounds tables are not
> +//sparse enough to make it worthwhile

bad comment style.

> +	unsigned char incore_vec[MPX_BOUNDS_TABLE_SIZE_BYTES/ PAGE_SIZE];
> + 	int incore_ret = mincore(bt_buf, MPX_BOUNDS_TABLE_SIZE_BYTES, &incore_vec[0]);

stray whitespace.

> +		//if (!incore_vec[page_nr])
> +		//	continue;

ugly.

> +		if (this_bt_entry_for_vaddr > 0x00007fffffffffffUL) {
> +			this_bt_entry_for_vaddr |= 0xffff800000000000;
> +		}

unnecessary curly braces.

> +		// Each bounds directory entry controls 1MB of
> +		// virtual address space.  This variable is the
> +		// virtual address in the process of the
> +		// beginning of the area controlled by this
> +		// bounds_dir.

ugly.

> +		unsigned long bd_for_vaddr = bd_index * (1UL<<20);
> +		//printf("%s() at bd index: %lx for vaddr: %lx\n", __func__, bd_index, bd_for_vaddr);
> +		//printf("dir entry[%4ld @ %p]\n", bd_index, bounds_dir_global+i);

Ditto.

> +		int nr_entries = dump_table(bounds_dir_entry, bd_for_vaddr, bounds_dir_global+bd_offset_bytes+i);
> +		total_entries += nr_entries;
> +		continue;
> +		printf("dir entry[%4ld @ %p]: 0x%lx %6d entries total this buf: %7d bd_for_vaddrs: 0x%lx -> 0x%lx\n",
> +				bd_index, buf+i,
> +				bounds_dir_entry, nr_entries, total_entries, bd_for_vaddr, bd_for_vaddr + (1UL<<20));

hm?

> +	// there shouldn't practically be short reads of /proc/$pid/mem

> +
> + 	incore_ret = mincore(dig_bounds_dir_ptr, buffer_size_bytes, &vec[0]);

stray space.

> +		//if (this_entries)
> +		//	printf("entries this loop: %d total: %d (bufs skipped: %d)\n", this_entries, total_entries, bufs_skipped);

You know the drill!

> +#ifdef MPX_DIG_REMOTE
> +int main(int argc, char **argv)
> +{
> +	int err;
> +        char *c;

hm...

etc. - ad nauseum. Would be nice to tidy this all up a bit.

Thanks,

	Ingo