Commit b5ffbd13 authored by Wen Yang's avatar Wen Yang Committed by Joel Granados
Browse files

sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array 



Move boundary checking for proc_dou8ved_minmax into module loading, thereby
reporting errors in advance. And add a kunit test case ensuring the
boundary check is done correctly.

The boundary check in proc_dou8vec_minmax done to the extra elements in
the ctl_table struct is currently performed at runtime. This allows buggy
kernel modules to be loaded normally without any errors only to fail
when used.

This is a buggy example module:
	#include <linux/kernel.h>
	#include <linux/module.h>
	#include <linux/sysctl.h>

	static struct ctl_table_header *_table_header = NULL;
	static unsigned char _data = 0;
	struct ctl_table table[] = {
		{
			.procname       = "foo",
			.data           = &_data,
			.maxlen         = sizeof(u8),
			.mode           = 0644,
			.proc_handler   = proc_dou8vec_minmax,
			.extra1         = SYSCTL_ZERO,
			.extra2         = SYSCTL_ONE_THOUSAND,
		},
	};

	static int init_demo(void) {
		_table_header = register_sysctl("kernel", table);
		if (!_table_header)
			return -ENOMEM;

		return 0;
	}

	module_init(init_demo);
	MODULE_LICENSE("GPL");

And this is the result:
        # insmod test.ko
        # cat /proc/sys/kernel/foo
        cat: /proc/sys/kernel/foo: Invalid argument

Suggested-by: default avatarJoel Granados <j.granados@samsung.com>
Signed-off-by: default avatarWen Yang <wen.yang@linux.dev>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-kernel@vger.kernel.org
Reviewed-by: default avatarJoel Granados <j.granados@samsung.com>
Signed-off-by: default avatarJoel Granados <j.granados@samsung.com>
parent 98ca62ba

fs/proc/proc_sysctl.c

+14 −0
Original line number Diff line number Diff line
@@ -1091,6 +1091,7 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...) 


static int sysctl_check_table_array(const char *path, struct ctl_table *table)

{

	unsigned int extra;

	int err = 0;



	if ((table->proc_handler == proc_douintvec) ||
@@ -1102,6 +1103,19 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table) 
	if (table->proc_handler == proc_dou8vec_minmax) {

		if (table->maxlen != sizeof(u8))

			err |= sysctl_err(path, table, "array not allowed");



		if (table->extra1) {

			extra = *(unsigned int *) table->extra1;

			if (extra > 255U)

				err |= sysctl_err(path, table,

						"range value too large for proc_dou8vec_minmax");

		}

		if (table->extra2) {

			extra = *(unsigned int *) table->extra2;

			if (extra > 255U)

				err |= sysctl_err(path, table,

						"range value too large for proc_dou8vec_minmax");

		}

	}



	if (table->proc_handler == proc_dobool) {

kernel/sysctl-test.c

+49 −0
Original line number Diff line number Diff line
@@ -367,6 +367,54 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( 
	KUNIT_EXPECT_EQ(test, 0, *((int *)table.data));

}



/*

 * Test that registering an invalid extra value is not allowed.

 */

static void sysctl_test_register_sysctl_sz_invalid_extra_value(

		struct kunit *test)

{

	unsigned char data = 0;

	struct ctl_table table_foo[] = {

		{

			.procname	= "foo",

			.data		= &data,

			.maxlen		= sizeof(u8),

			.mode		= 0644,

			.proc_handler	= proc_dou8vec_minmax,

			.extra1		= SYSCTL_FOUR,

			.extra2		= SYSCTL_ONE_THOUSAND,

		},

	};



	struct ctl_table table_bar[] = {

		{

			.procname	= "bar",

			.data		= &data,

			.maxlen		= sizeof(u8),

			.mode		= 0644,

			.proc_handler	= proc_dou8vec_minmax,

			.extra1		= SYSCTL_NEG_ONE,

			.extra2		= SYSCTL_ONE_HUNDRED,

		},

	};



	struct ctl_table table_qux[] = {

		{

			.procname	= "qux",

			.data		= &data,

			.maxlen		= sizeof(u8),

			.mode		= 0644,

			.proc_handler	= proc_dou8vec_minmax,

			.extra1		= SYSCTL_ZERO,

			.extra2		= SYSCTL_TWO_HUNDRED,

		},

	};



	KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_foo));

	KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_bar));

	KUNIT_EXPECT_NOT_NULL(test, register_sysctl("foo", table_qux));

}



static struct kunit_case sysctl_test_cases[] = {

	KUNIT_CASE(sysctl_test_api_dointvec_null_tbl_data),

	KUNIT_CASE(sysctl_test_api_dointvec_table_maxlen_unset),
@@ -378,6 +426,7 @@ static struct kunit_case sysctl_test_cases[] = { 
	KUNIT_CASE(sysctl_test_dointvec_write_happy_single_negative),

	KUNIT_CASE(sysctl_test_api_dointvec_write_single_less_int_min),

	KUNIT_CASE(sysctl_test_api_dointvec_write_single_greater_int_max),

	KUNIT_CASE(sysctl_test_register_sysctl_sz_invalid_extra_value),

	{}

};

kernel/sysctl.c

+2 −8
Original line number Diff line number Diff line
@@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, int write, 
	if (table->maxlen != sizeof(u8))

		return -EINVAL;



	if (table->extra1) {

	if (table->extra1)

		min = *(unsigned int *) table->extra1;

		if (min > 255U)

			return -EINVAL;

	}

	if (table->extra2) {

	if (table->extra2)

		max = *(unsigned int *) table->extra2;

		if (max > 255U)

			return -EINVAL;

	}



	tmp = *table;