Re: [PATCH v2] mm: add arch-independent testcases for RODATA

From: Russell King - ARM Linux
Date: Thu Jan 19 2017 - 10:47:55 EST


On Fri, Jan 20, 2017 at 12:39:20AM +0900, Jinbum Park wrote:
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index afcc550..e4f22ce 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -90,3 +90,9 @@ config DEBUG_PAGE_REF
> careful when enabling this feature because it adds about 30 KB to the
> kernel code. However the runtime performance overhead is virtually
> nil until the tracepoints are actually enabled.
> +
> +config DEBUG_RODATA_TEST
> + bool "Testcase for the marking rodata read-only"
> + depends on DEBUG_RODATA
> + ---help---
> + This option enables a testcase for the setting rodata read-only.
> \ No newline at end of file

It's worth reviewing your own patches before sending them out for
things like this (please ensure that all files are not left without
a newline at the end.)

> diff --git a/mm/rodata_test.c b/mm/rodata_test.c
> new file mode 100644
> index 0000000..fb953c0
> --- /dev/null
> +++ b/mm/rodata_test.c
> @@ -0,0 +1,64 @@
> +/*
> + * rodata_test.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2008 Intel Corporation
> + * Author: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> + *
> + * 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; version 2
> + * of the License.
> + */
> +#include <asm/uaccess.h>
> +#include <asm/sections.h>
> +
> +const int rodata_test_data = 0xC3;
> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +
> +void rodata_test(void)
> +{
> + unsigned long start, end, rodata_addr;
> + int zero = 0;
> +
> + /* prepare test */
> + rodata_addr = ((unsigned long)&rodata_test_data);
> +
> + /* test 1: read the value */
> + /* If this test fails, some previous testrun has clobbered the state */
> + if (!rodata_test_data) {
> + pr_err("rodata_test: test 1 fails (start data)\n");
> + return;
> + }
> +
> + /* test 2: write to the variable; this should fault */
> + /*
> + * This must be written in assembly to be able to catch the
> + * exception that is supposed to happen in the correct case.
> + *
> + * So that put_user macro is used to write arch-independent assembly.
> + */
> + if (!put_user(zero, (int *)rodata_addr)) {
> + pr_err("rodata_test: test data was not read only\n");
> + return;
> + }

I don't think this is going to do what you think - at least not on sane
architectures. put_user() to kernel space is denied, even if the
location is writable to normal accesses within the kernel.

put_user() and get_user() are for accessing user supplied pointers,
which means it has built-in security to prevent userspace passing in
kernel-space pointers and using that as a way to read or modify kernel
space.

I think you want to use probe_kernel_write() here.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.