From: Wen Yang <[email protected]>
The boundary check of u8's extra is currently performed at runtime.
This may result in some kernel modules that can be loaded normally without
any errors, but not works, as follows:
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/sysctl.h>
static struct ctl_table_header *_table_header;
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) {
if (!_table_header)
_table_header = register_sysctl("kernel", table);
pr_info("test: init module.\n");
return 0;
}
static void cleanup_demo(void) {
if (_table_header) {
unregister_sysctl_table(_table_header);
}
pr_info("test: cleanup module.\n");
}
module_init(init_demo);
module_exit(cleanup_demo);
MODULE_LICENSE("GPL");
# insmod test.ko
# cat /proc/sys/kernel/foo
cat: /proc/sys/kernel/foo: Invalid argument
# echo 1 > /proc/sys/kernel/foo
-bash: echo: write error: Invalid argument
This patch moves the boundary check forward to module loading and
also adds a kunit test case.
Signed-off-by: Wen Yang <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Joel Granados <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: [email protected]
---
v2->v1:
- kunit: detect registration failure with KUNIT_EXPECT_NULL
fs/proc/proc_sysctl.c | 12 ++++++++++++
kernel/sysctl-test.c | 30 ++++++++++++++++++++++++++++++
kernel/sysctl.c | 14 ++++----------
3 files changed, 46 insertions(+), 10 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 37cde0efee57..136e3f8966c3 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1096,6 +1096,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) ||
@@ -1107,6 +1108,17 @@ 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, "array not allowed");
+ }
+ if (table->extra2) {
+ extra = *(unsigned int *) table->extra2;
+ if (extra > 255U)
+ err |= sysctl_err(path, table, "array not allowed");
+ }
}
if (table->proc_handler == proc_dobool) {
diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
index 6ef887c19c48..84e759a8328f 100644
--- a/kernel/sysctl-test.c
+++ b/kernel/sysctl-test.c
@@ -367,6 +367,35 @@ 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[] = {
+ {
+ .procname = "foo",
+ .data = &data,
+ .maxlen = sizeof(u8),
+ .mode = 0644,
+ .proc_handler = proc_dou8vec_minmax,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE_THOUSAND,
+ },
+ {}
+ };
+ unsigned int size = ARRAY_SIZE(table);
+
+ KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
+ table[0].extra1 = SYSCTL_ONE_THOUSAND;
+ KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
+ table[0].extra1 = SYSCTL_ONE_HUNDRED;
+ table[0].extra2 = SYSCTL_TWO_HUNDRED;
+ KUNIT_EXPECT_NOT_NULL(test, register_sysctl_sz("foo", table, size));
+}
+
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 +407,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),
{}
};
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f67b39d3d6e5..28888744626a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, int write,
if (table->maxlen != sizeof(u8))
return -EINVAL;
- if (table->extra1) {
- min = *(unsigned int *) table->extra1;
- if (min > 255U)
- return -EINVAL;
- }
- if (table->extra2) {
- max = *(unsigned int *) table->extra2;
- if (max > 255U)
- return -EINVAL;
- }
+ if (table->extra1)
+ min = *(unsigned char *) table->extra1;
+ if (table->extra2)
+ max = *(unsigned char *) table->extra2;
tmp = *table;
--
2.25.1
On Sat, Mar 02, 2024 at 11:27:45PM +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>
<--- snip --->
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> index 6ef887c19c48..84e759a8328f 100644
> --- a/kernel/sysctl-test.c
> +++ b/kernel/sysctl-test.c
> @@ -367,6 +367,35 @@ 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[] = {
I'm pretty sure that this is going to clash with the constification that
Thomas is working on. Please re-work this patch knowing that these
ctl_tables are going to have to change to const at some point.
> + {
> + .procname = "foo",
> + .data = &data,
> + .maxlen = sizeof(u8),
> + .mode = 0644,
> + .proc_handler = proc_dou8vec_minmax,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = SYSCTL_ONE_THOUSAND,
> + },
> + {}
Don't use the sentinel here. We are removing them and all new sysctl
tables (even the test ones) should be created without them
> + };
> + unsigned int size = ARRAY_SIZE(table);
You do not need size here. When you use register_sysctl, the size will
be automatically calculated for you.
> +
> + KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
> + table[0].extra1 = SYSCTL_ONE_THOUSAND;
> + KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
> + table[0].extra1 = SYSCTL_ONE_HUNDRED;
> + table[0].extra2 = SYSCTL_TWO_HUNDRED;
> + KUNIT_EXPECT_NOT_NULL(test, register_sysctl_sz("foo", table, size));
Replace all these by register_sysctl please.
> +}
> +
> 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 +407,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),
> {}
> };
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f67b39d3d6e5..28888744626a 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, int write,
> if (table->maxlen != sizeof(u8))
> return -EINVAL;
>
> - if (table->extra1) {
> - min = *(unsigned int *) table->extra1;
> - if (min > 255U)
> - return -EINVAL;
> - }
> - if (table->extra2) {
> - max = *(unsigned int *) table->extra2;
> - if (max > 255U)
> - return -EINVAL;
> - }
> + if (table->extra1)
> + min = *(unsigned char *) table->extra1;
> + if (table->extra2)
> + max = *(unsigned char *) table->extra2;
>
> tmp = *table;
>
> --
> 2.25.1
>
--
Joel Granados
On 2024/3/29 00:53, Joel Granados wrote:
> On Sat, Mar 02, 2024 at 11:27:45PM +0800, [email protected] wrote:
>> From: Wen Yang <[email protected]>
>>
>
> <--- snip --->
>
>> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
>> index 6ef887c19c48..84e759a8328f 100644
>> --- a/kernel/sysctl-test.c
>> +++ b/kernel/sysctl-test.c
>> @@ -367,6 +367,35 @@ 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[] = {
> I'm pretty sure that this is going to clash with the constification that
> Thomas is working on. Please re-work this patch knowing that these
> ctl_tables are going to have to change to const at some point.
>
>> + {
>> + .procname = "foo",
>> + .data = &data,
>> + .maxlen = sizeof(u8),
>> + .mode = 0644,
>> + .proc_handler = proc_dou8vec_minmax,
>> + .extra1 = SYSCTL_ZERO,
>> + .extra2 = SYSCTL_ONE_THOUSAND,
>> + },
>> + {}
> Don't use the sentinel here. We are removing them and all new sysctl
> tables (even the test ones) should be created without them
>
>> + };
>> + unsigned int size = ARRAY_SIZE(table);
> You do not need size here. When you use register_sysctl, the size will
> be automatically calculated for you.
>
>> +
>> + KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
>> + table[0].extra1 = SYSCTL_ONE_THOUSAND;
>> + KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
>> + table[0].extra1 = SYSCTL_ONE_HUNDRED;
>> + table[0].extra2 = SYSCTL_TWO_HUNDRED;
>> + KUNIT_EXPECT_NOT_NULL(test, register_sysctl_sz("foo", table, size));
> Replace all these by register_sysctl please.
>
Thanks for your review comments w.r.t [PATCH v2], which will be complied
by in next patch version [PATCH v3].
--
Best wishes,
Wen
>> +}
>> +
>> 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 +407,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),
>> {}
>> };
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index f67b39d3d6e5..28888744626a 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, int write,
>> if (table->maxlen != sizeof(u8))
>> return -EINVAL;
>>
>> - if (table->extra1) {
>> - min = *(unsigned int *) table->extra1;
>> - if (min > 255U)
>> - return -EINVAL;
>> - }
>> - if (table->extra2) {
>> - max = *(unsigned int *) table->extra2;
>> - if (max > 255U)
>> - return -EINVAL;
>> - }
>> + if (table->extra1)
>> + min = *(unsigned char *) table->extra1;
>> + if (table->extra2)
>> + max = *(unsigned char *) table->extra2;
>>
>> tmp = *table;
>>
>> --
>> 2.25.1
>>
>