* [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 20:13 ` Kees Cook
2017-02-11 0:36 ` [PATCH v2 2/9] sysctl: add proper unsigned int support Luis R. Rodriguez
` (9 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
improved sanity checks considerbly, however the enhancements on
sysctl_check_table() meant adding a functional change so that
only the last table entry's sanity error is propagated. It also
changed the way errors were propagated so that each new check
reset the err value, this means only last sanity check computed
is used for an error. This has been in the kernel since v3.4 days.
Fix this by carrying on errors from previous checks and iterations
as we traverse the table and ensuring we keep any error from previous
checks. We keep iterating on the table even if an error is found so
we can complain for all errors found in one shot. This works as
-EINVAL is always returned on error anyway, and the check for error
is any non-zero value.
Fixes: 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
fs/proc/proc_sysctl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index d4e37acd4821..d22ee738d2eb 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1036,7 +1036,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
int err = 0;
for (; table->procname; table++) {
if (table->child)
- err = sysctl_err(path, table, "Not a file");
+ err |= sysctl_err(path, table, "Not a file");
if ((table->proc_handler == proc_dostring) ||
(table->proc_handler == proc_dointvec) ||
@@ -1047,15 +1047,15 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
(table->proc_handler == proc_doulongvec_minmax) ||
(table->proc_handler == proc_doulongvec_ms_jiffies_minmax)) {
if (!table->data)
- err = sysctl_err(path, table, "No data");
+ err |= sysctl_err(path, table, "No data");
if (!table->maxlen)
- err = sysctl_err(path, table, "No maxlen");
+ err |= sysctl_err(path, table, "No maxlen");
}
if (!table->proc_handler)
- err = sysctl_err(path, table, "No proc_handler");
+ err |= sysctl_err(path, table, "No proc_handler");
if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
- err = sysctl_err(path, table, "bogus .mode 0%o",
+ err |= sysctl_err(path, table, "bogus .mode 0%o",
table->mode);
}
return err;
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check
2017-02-11 0:36 ` [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
@ 2017-02-13 20:13 ` Kees Cook
0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2017-02-13 20:13 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
> improved sanity checks considerbly, however the enhancements on
> sysctl_check_table() meant adding a functional change so that
> only the last table entry's sanity error is propagated. It also
> changed the way errors were propagated so that each new check
> reset the err value, this means only last sanity check computed
> is used for an error. This has been in the kernel since v3.4 days.
>
> Fix this by carrying on errors from previous checks and iterations
> as we traverse the table and ensuring we keep any error from previous
> checks. We keep iterating on the table even if an error is found so
> we can complain for all errors found in one shot. This works as
> -EINVAL is always returned on error anyway, and the check for error
> is any non-zero value.
I assume this didn't catch anything that was broken-but-hidden in the kernel?
>
> Fixes: 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 2/9] sysctl: add proper unsigned int support
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
2017-02-11 0:36 ` [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 20:19 ` Kees Cook
2017-02-11 0:36 ` [PATCH v2 3/9] sysctl: add unsigned int range support Luis R. Rodriguez
` (8 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez, Heinrich Schuchardt,
David S. Miller, Ingo Molnar
Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32
fields") added proc_douintvec() to start help adding support for
unsigned int, this however was only half the work needed, all these
issues are present with the current implementation:
o Printing the values shows a negative value, this happens since
do_proc_dointvec() and this uses proc_put_long()
o We can easily wrap around the int values: UINT_MAX is 4294967295,
if we echo in 4294967295 + 1 we end up with 0, using 4294967295 + 2
we end up with 1.
o We echo negative values in and they are accepted
o sysctl_check_table() was never extended for proc_douintvec()
Fix all these issues by adding our own do_proc_douintvec() and adding
proc_douintvec() to sysctl_check_table().
Historically sysctl proc helpers have supported arrays, due to the
complexity this adds though we've taken a step back to evaluate array
users to determine if its worth upkeeping for unsigned int. An
evaluation using Coccinelle has been done to perform a grammatical
search to ask ourselves:
o How many sysctl proc_dointvec() (int) users exist which likely
should be moved over to proc_douintvec() (unsigned int) ?
Answer: about 8
- Of these how many are array users ?
Answer: Probably only 1
o How many sysctl array users exist ?
Answer: about 12
This last question gives us an idea just how popular arrays: they
are not. Array support should probably just be kept for strings.
The identified uint ports are:
drivers/infiniband/core/ucma.c - max_backlog
drivers/infiniband/core/iwcm.c - default_backlog
net/core/sysctl_net_core.c - rps_sock_flow_sysctl()
net/netfilter/nf_conntrack_timestamp.c - nf_conntrack_timestamp -- bool
net/netfilter/nf_conntrack_acct.c nf_conntrack_acct -- bool
net/netfilter/nf_conntrack_ecache.c - nf_conntrack_events -- bool
net/netfilter/nf_conntrack_helper.c - nf_conntrack_helper -- bool
net/phonet/sysctl.c proc_local_port_range()
The only possible array users is proc_local_port_range() but it does not
seem worth it to add array support just for this given the range support
works just as well. Unsigned int support should be desirable more for
when you *need* more than INT_MAX or using int min/max support then
does not suffice for your ranges.
If you forget and by mistake happen to register an unsigned int proc entry
with an array, the driver will fail and you will get something as follows:
sysctl table check failed: debug/test_sysctl//uint_0002 array now allowed
CPU: 2 PID: 1342 Comm: modprobe Tainted: G W E <etc>
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <etc>
Call Trace:
dump_stack+0x63/0x81
__register_sysctl_table+0x350/0x650
? kmem_cache_alloc_trace+0x107/0x240
__register_sysctl_paths+0x1b3/0x1e0
? 0xffffffffc005f000
register_sysctl_table+0x1f/0x30
test_sysctl_init+0x10/0x1000 [test_sysctl]
do_one_initcall+0x52/0x1a0
? kmem_cache_alloc_trace+0x107/0x240
do_init_module+0x5f/0x200
load_module+0x1867/0x1bd0
? __symbol_put+0x60/0x60
SYSC_finit_module+0xdf/0x110
SyS_finit_module+0xe/0x10
entry_SYSCALL_64_fastpath+0x1e/0xad
RIP: 0033:0x7f042b22d119
<etc>
Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
fs/proc/proc_sysctl.c | 15 +++++
kernel/sysctl.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 170 insertions(+), 6 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index d22ee738d2eb..73696a73a1ec 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1031,6 +1031,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
return -EINVAL;
}
+static int sysctl_check_table_array(const char *path, struct ctl_table *table)
+{
+ int err = 0;
+
+ if (table->proc_handler == proc_douintvec) {
+ if (table->maxlen != sizeof(unsigned int))
+ err |= sysctl_err(path, table, "array now allowed");
+ }
+
+ return err;
+}
+
static int sysctl_check_table(const char *path, struct ctl_table *table)
{
int err = 0;
@@ -1040,6 +1052,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
if ((table->proc_handler == proc_dostring) ||
(table->proc_handler == proc_dointvec) ||
+ (table->proc_handler == proc_douintvec) ||
(table->proc_handler == proc_dointvec_minmax) ||
(table->proc_handler == proc_dointvec_jiffies) ||
(table->proc_handler == proc_dointvec_userhz_jiffies) ||
@@ -1050,6 +1063,8 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
err |= sysctl_err(path, table, "No data");
if (!table->maxlen)
err |= sysctl_err(path, table, "No maxlen");
+ else
+ err |= sysctl_check_table_array(path, table);
}
if (!table->proc_handler)
err |= sysctl_err(path, table, "No proc_handler");
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 1aea594a54db..493bc05e546a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
return 0;
}
-static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
- int *valp,
- int write, void *data)
+static int do_proc_douintvec_conv(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data)
{
if (write) {
- if (*negp)
+ if (*lvalp > UINT_MAX)
return -EINVAL;
*valp = *lvalp;
} else {
@@ -2243,6 +2243,155 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
buffer, lenp, ppos, conv, data);
}
+static int do_proc_douintvec_w(unsigned int *tbl_data,
+ struct ctl_table *table,
+ void __user *buffer,
+ size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ unsigned long lval;
+ int err = 0;
+ size_t left;
+ bool neg;
+ char *kbuf = NULL, *p;
+
+ left = *lenp;
+
+ if (*ppos) {
+ switch (sysctl_writes_strict) {
+ case SYSCTL_WRITES_STRICT:
+ goto bail_early;
+ case SYSCTL_WRITES_WARN:
+ warn_sysctl_write(table);
+ break;
+ default:
+ break;
+ }
+ }
+
+ if (left > PAGE_SIZE - 1)
+ left = PAGE_SIZE - 1;
+
+ p = kbuf = memdup_user_nul(buffer, left);
+ if (IS_ERR(kbuf))
+ return -EINVAL;
+
+ left -= proc_skip_spaces(&p);
+ if (!left) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
+ err = proc_get_long(&p, &left, &lval, &neg,
+ proc_wspace_sep,
+ sizeof(proc_wspace_sep), NULL);
+ if (err || neg) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
+ if (conv(&lval, tbl_data, 1, data)) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
+ if (!err && left)
+ left -= proc_skip_spaces(&p);
+
+out_free:
+ kfree(kbuf);
+ if (err)
+ return -EINVAL;
+
+ return 0;
+
+ /* This is in keeping with old __do_proc_dointvec() */
+bail_early:
+ *ppos += *lenp;
+ return err;
+}
+
+static int do_proc_douintvec_r(unsigned int *tbl_data, void __user *buffer,
+ size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ unsigned long lval;
+ int err = 0;
+ size_t left;
+
+ left = *lenp;
+
+ if (conv(&lval, tbl_data, 0, data)) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ err = proc_put_long(&buffer, &left, lval, false);
+ if (err || !left)
+ goto out;
+
+ err = proc_put_char(&buffer, &left, '\n');
+
+out:
+ *lenp -= left;
+ *ppos += *lenp;
+
+ return err;
+}
+
+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
+ int write, void __user *buffer,
+ size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ unsigned int *i, vleft;
+
+ if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
+ *lenp = 0;
+ return 0;
+ }
+
+ i = (unsigned int *) tbl_data;
+ vleft = table->maxlen / sizeof(*i);
+
+ /*
+ * Arrays are not supported, keep this simple. *Do not* add
+ * support for them.
+ */
+ if (vleft != 1) {
+ *lenp = 0;
+ return -EINVAL;
+ }
+
+ if (!conv)
+ conv = do_proc_douintvec_conv;
+
+ if (write)
+ return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
+ conv, data);
+ return do_proc_douintvec_r(i, buffer, lenp, ppos, conv, data);
+}
+
+static int do_proc_douintvec(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ return __do_proc_douintvec(table->data, table, write,
+ buffer, lenp, ppos, conv, data);
+}
+
/**
* proc_dointvec - read a vector of integers
* @table: the sysctl table
@@ -2278,8 +2427,8 @@ int proc_dointvec(struct ctl_table *table, int write,
int proc_douintvec(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- return do_proc_dointvec(table, write, buffer, lenp, ppos,
- do_proc_douintvec_conv, NULL);
+ return do_proc_douintvec(table, write, buffer, lenp, ppos,
+ do_proc_douintvec_conv, NULL);
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 2/9] sysctl: add proper unsigned int support
2017-02-11 0:36 ` [PATCH v2 2/9] sysctl: add proper unsigned int support Luis R. Rodriguez
@ 2017-02-13 20:19 ` Kees Cook
2017-05-16 22:25 ` Luis R. Rodriguez
0 siblings, 1 reply; 34+ messages in thread
From: Kees Cook @ 2017-02-13 20:19 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML, Heinrich Schuchardt,
David S. Miller, Ingo Molnar
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32
> fields") added proc_douintvec() to start help adding support for
> unsigned int, this however was only half the work needed, all these
> issues are present with the current implementation:
>
> o Printing the values shows a negative value, this happens since
> do_proc_dointvec() and this uses proc_put_long()
> o We can easily wrap around the int values: UINT_MAX is 4294967295,
> if we echo in 4294967295 + 1 we end up with 0, using 4294967295 + 2
> we end up with 1.
> o We echo negative values in and they are accepted
> o sysctl_check_table() was never extended for proc_douintvec()
>
> Fix all these issues by adding our own do_proc_douintvec() and adding
> proc_douintvec() to sysctl_check_table().
>
> Historically sysctl proc helpers have supported arrays, due to the
> complexity this adds though we've taken a step back to evaluate array
> users to determine if its worth upkeeping for unsigned int. An
> evaluation using Coccinelle has been done to perform a grammatical
> search to ask ourselves:
>
> o How many sysctl proc_dointvec() (int) users exist which likely
> should be moved over to proc_douintvec() (unsigned int) ?
> Answer: about 8
> - Of these how many are array users ?
> Answer: Probably only 1
> o How many sysctl array users exist ?
> Answer: about 12
>
> This last question gives us an idea just how popular arrays: they
> are not. Array support should probably just be kept for strings.
>
> The identified uint ports are:
>
> drivers/infiniband/core/ucma.c - max_backlog
> drivers/infiniband/core/iwcm.c - default_backlog
> net/core/sysctl_net_core.c - rps_sock_flow_sysctl()
> net/netfilter/nf_conntrack_timestamp.c - nf_conntrack_timestamp -- bool
> net/netfilter/nf_conntrack_acct.c nf_conntrack_acct -- bool
> net/netfilter/nf_conntrack_ecache.c - nf_conntrack_events -- bool
> net/netfilter/nf_conntrack_helper.c - nf_conntrack_helper -- bool
> net/phonet/sysctl.c proc_local_port_range()
>
> The only possible array users is proc_local_port_range() but it does not
> seem worth it to add array support just for this given the range support
> works just as well. Unsigned int support should be desirable more for
> when you *need* more than INT_MAX or using int min/max support then
> does not suffice for your ranges.
>
> If you forget and by mistake happen to register an unsigned int proc entry
> with an array, the driver will fail and you will get something as follows:
>
> sysctl table check failed: debug/test_sysctl//uint_0002 array now allowed
> CPU: 2 PID: 1342 Comm: modprobe Tainted: G W E <etc>
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <etc>
> Call Trace:
> dump_stack+0x63/0x81
> __register_sysctl_table+0x350/0x650
> ? kmem_cache_alloc_trace+0x107/0x240
> __register_sysctl_paths+0x1b3/0x1e0
> ? 0xffffffffc005f000
> register_sysctl_table+0x1f/0x30
> test_sysctl_init+0x10/0x1000 [test_sysctl]
> do_one_initcall+0x52/0x1a0
> ? kmem_cache_alloc_trace+0x107/0x240
> do_init_module+0x5f/0x200
> load_module+0x1867/0x1bd0
> ? __symbol_put+0x60/0x60
> SYSC_finit_module+0xdf/0x110
> SyS_finit_module+0xe/0x10
> entry_SYSCALL_64_fastpath+0x1e/0xad
> RIP: 0033:0x7f042b22d119
> <etc>
>
> Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> fs/proc/proc_sysctl.c | 15 +++++
> kernel/sysctl.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 170 insertions(+), 6 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index d22ee738d2eb..73696a73a1ec 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1031,6 +1031,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
> return -EINVAL;
> }
>
> +static int sysctl_check_table_array(const char *path, struct ctl_table *table)
> +{
> + int err = 0;
> +
> + if (table->proc_handler == proc_douintvec) {
Should this be inverted? i.e. explicitly allow proc_handlers instead
of only rejecting douintvec?
> + if (table->maxlen != sizeof(unsigned int))
> + err |= sysctl_err(path, table, "array now allowed");
> + }
> +
> + return err;
> +}
> +
> static int sysctl_check_table(const char *path, struct ctl_table *table)
> {
> int err = 0;
> @@ -1040,6 +1052,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
>
> if ((table->proc_handler == proc_dostring) ||
> (table->proc_handler == proc_dointvec) ||
> + (table->proc_handler == proc_douintvec) ||
> (table->proc_handler == proc_dointvec_minmax) ||
> (table->proc_handler == proc_dointvec_jiffies) ||
> (table->proc_handler == proc_dointvec_userhz_jiffies) ||
> @@ -1050,6 +1063,8 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
> err |= sysctl_err(path, table, "No data");
> if (!table->maxlen)
> err |= sysctl_err(path, table, "No maxlen");
> + else
> + err |= sysctl_check_table_array(path, table);
> }
> if (!table->proc_handler)
> err |= sysctl_err(path, table, "No proc_handler");
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 1aea594a54db..493bc05e546a 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
> return 0;
> }
>
> -static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
> - int *valp,
> - int write, void *data)
> +static int do_proc_douintvec_conv(unsigned long *lvalp,
> + unsigned int *valp,
> + int write, void *data)
> {
> if (write) {
> - if (*negp)
> + if (*lvalp > UINT_MAX)
> return -EINVAL;
> *valp = *lvalp;
> } else {
> @@ -2243,6 +2243,155 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
> buffer, lenp, ppos, conv, data);
> }
>
> +static int do_proc_douintvec_w(unsigned int *tbl_data,
> + struct ctl_table *table,
> + void __user *buffer,
> + size_t *lenp, loff_t *ppos,
> + int (*conv)(unsigned long *lvalp,
> + unsigned int *valp,
> + int write, void *data),
> + void *data)
> +{
> + unsigned long lval;
> + int err = 0;
> + size_t left;
> + bool neg;
> + char *kbuf = NULL, *p;
> +
> + left = *lenp;
> +
> + if (*ppos) {
> + switch (sysctl_writes_strict) {
> + case SYSCTL_WRITES_STRICT:
> + goto bail_early;
> + case SYSCTL_WRITES_WARN:
> + warn_sysctl_write(table);
> + break;
> + default:
> + break;
> + }
> + }
I wonder if this SYSCTL_WRITES_* test copy/pasting needs to be a function?
> +
> + if (left > PAGE_SIZE - 1)
> + left = PAGE_SIZE - 1;
> +
> + p = kbuf = memdup_user_nul(buffer, left);
> + if (IS_ERR(kbuf))
> + return -EINVAL;
> +
> + left -= proc_skip_spaces(&p);
> + if (!left) {
> + err = -EINVAL;
> + goto out_free;
> + }
> +
> + err = proc_get_long(&p, &left, &lval, &neg,
> + proc_wspace_sep,
> + sizeof(proc_wspace_sep), NULL);
> + if (err || neg) {
> + err = -EINVAL;
> + goto out_free;
> + }
> +
> + if (conv(&lval, tbl_data, 1, data)) {
> + err = -EINVAL;
> + goto out_free;
> + }
> +
> + if (!err && left)
> + left -= proc_skip_spaces(&p);
> +
> +out_free:
> + kfree(kbuf);
> + if (err)
> + return -EINVAL;
> +
> + return 0;
> +
> + /* This is in keeping with old __do_proc_dointvec() */
> +bail_early:
> + *ppos += *lenp;
> + return err;
> +}
> +
> +static int do_proc_douintvec_r(unsigned int *tbl_data, void __user *buffer,
> + size_t *lenp, loff_t *ppos,
> + int (*conv)(unsigned long *lvalp,
> + unsigned int *valp,
> + int write, void *data),
> + void *data)
> +{
> + unsigned long lval;
> + int err = 0;
> + size_t left;
> +
> + left = *lenp;
> +
> + if (conv(&lval, tbl_data, 0, data)) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + err = proc_put_long(&buffer, &left, lval, false);
> + if (err || !left)
> + goto out;
> +
> + err = proc_put_char(&buffer, &left, '\n');
> +
> +out:
> + *lenp -= left;
> + *ppos += *lenp;
> +
> + return err;
> +}
> +
> +static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
> + int write, void __user *buffer,
> + size_t *lenp, loff_t *ppos,
> + int (*conv)(unsigned long *lvalp,
> + unsigned int *valp,
> + int write, void *data),
> + void *data)
> +{
> + unsigned int *i, vleft;
> +
> + if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
> + *lenp = 0;
> + return 0;
> + }
> +
> + i = (unsigned int *) tbl_data;
> + vleft = table->maxlen / sizeof(*i);
> +
> + /*
> + * Arrays are not supported, keep this simple. *Do not* add
> + * support for them.
> + */
> + if (vleft != 1) {
> + *lenp = 0;
> + return -EINVAL;
> + }
> +
> + if (!conv)
> + conv = do_proc_douintvec_conv;
> +
> + if (write)
> + return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
> + conv, data);
> + return do_proc_douintvec_r(i, buffer, lenp, ppos, conv, data);
> +}
I like the split of read/write. That makes things easier to review.
> +
> +static int do_proc_douintvec(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos,
> + int (*conv)(unsigned long *lvalp,
> + unsigned int *valp,
> + int write, void *data),
> + void *data)
> +{
> + return __do_proc_douintvec(table->data, table, write,
> + buffer, lenp, ppos, conv, data);
> +}
> +
> /**
> * proc_dointvec - read a vector of integers
> * @table: the sysctl table
> @@ -2278,8 +2427,8 @@ int proc_dointvec(struct ctl_table *table, int write,
> int proc_douintvec(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
> - return do_proc_dointvec(table, write, buffer, lenp, ppos,
> - do_proc_douintvec_conv, NULL);
> + return do_proc_douintvec(table, write, buffer, lenp, ppos,
> + do_proc_douintvec_conv, NULL);
> }
>
> /*
> --
> 2.11.0
>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2 2/9] sysctl: add proper unsigned int support
2017-02-13 20:19 ` Kees Cook
@ 2017-05-16 22:25 ` Luis R. Rodriguez
0 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-16 22:25 UTC (permalink / raw)
To: Kees Cook
Cc: Luis R. Rodriguez, Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML, Heinrich Schuchardt,
David S. Miller, Ingo Molnar
On Mon, Feb 13, 2017 at 12:19:51PM -0800, Kees Cook wrote:
> On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > ---
> > fs/proc/proc_sysctl.c | 15 +++++
> > kernel/sysctl.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 170 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> > index d22ee738d2eb..73696a73a1ec 100644
> > --- a/fs/proc/proc_sysctl.c
> > +++ b/fs/proc/proc_sysctl.c
> > @@ -1031,6 +1031,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
> > return -EINVAL;
> > }
> >
> > +static int sysctl_check_table_array(const char *path, struct ctl_table *table)
> > +{
> > + int err = 0;
> > +
> > + if (table->proc_handler == proc_douintvec) {
>
> Should this be inverted? i.e. explicitly allow proc_handlers instead
> of only rejecting douintvec?
The goal is to start avoiding the use of the silly arrays, and we start drawing
the line with proc_douintvec. The expectation then is that this list will grow
so the check should easily allow for growth of more exclusions for now, once we
are done we would hopefully only end up with strings that use arrays. For now
then we are limited to a specific check against the size of the parameter that
the proc handler uses, so for instance:
> > + if (table->maxlen != sizeof(unsigned int))
> > + err |= sysctl_err(path, table, "array now allowed");
> > + }
This uses unsigned int, I expect the check to be different for proc_dointvec_jiffies
once we vet no array uses exist for it. The way I think its best to expand on this
list is to later add:
else if (table->proc_handler == proc_dointvec_jiffies) {
...
}
If did the other way around and started this check with:
if (table->proc_handler != proc_douintvec)
return 0;
We'd still have to add a specific check for each handler for the actual expected
size for one element. So I would prefer to keep it this way for now. Once we have
vetted arrays are only needed for strings (and hopefully if we can change a few
ints users, not sure if proc is "uapi"; think it is so we might be shit out of luck),
then we'd only have a short white-list. Even so, the size check on the others is
probably good to leave, as such a check does not exist so we would not have to
nuke those old checks.
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 1aea594a54db..493bc05e546a 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -2243,6 +2243,155 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
> > buffer, lenp, ppos, conv, data);
> > }
> >
> > +static int do_proc_douintvec_w(unsigned int *tbl_data,
> > + struct ctl_table *table,
> > + void __user *buffer,
> > + size_t *lenp, loff_t *ppos,
> > + int (*conv)(unsigned long *lvalp,
> > + unsigned int *valp,
> > + int write, void *data),
> > + void *data)
> > +{
> > + unsigned long lval;
> > + int err = 0;
> > + size_t left;
> > + bool neg;
> > + char *kbuf = NULL, *p;
> > +
> > + left = *lenp;
> > +
> > + if (*ppos) {
> > + switch (sysctl_writes_strict) {
> > + case SYSCTL_WRITES_STRICT:
> > + goto bail_early;
> > + case SYSCTL_WRITES_WARN:
> > + warn_sysctl_write(table);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
>
> I wonder if this SYSCTL_WRITES_* test copy/pasting needs to be a function?
Good call. Folded in a new patch that does this. This stuff was a bit cryptic
so also folded in another patch which documented the strict stuff a bit in
kdoc form. Later with time we can move to new sphinx doc format and take
advantage of that.
<-- snip -->
> > +static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
> > + int write, void __user *buffer,
> > + size_t *lenp, loff_t *ppos,
> > + int (*conv)(unsigned long *lvalp,
> > + unsigned int *valp,
> > + int write, void *data),
> > + void *data)
> > +{
> > + unsigned int *i, vleft;
> > +
> > + if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
> > + *lenp = 0;
> > + return 0;
> > + }
> > +
> > + i = (unsigned int *) tbl_data;
> > + vleft = table->maxlen / sizeof(*i);
> > +
> > + /*
> > + * Arrays are not supported, keep this simple. *Do not* add
> > + * support for them.
> > + */
> > + if (vleft != 1) {
> > + *lenp = 0;
> > + return -EINVAL;
> > + }
> > +
> > + if (!conv)
> > + conv = do_proc_douintvec_conv;
> > +
> > + if (write)
> > + return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
> > + conv, data);
> > + return do_proc_douintvec_r(i, buffer, lenp, ppos, conv, data);
> > +}
>
> I like the split of read/write. That makes things easier to review.
Great.
Luis
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 3/9] sysctl: add unsigned int range support
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
2017-02-11 0:36 ` [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
2017-02-11 0:36 ` [PATCH v2 2/9] sysctl: add proper unsigned int support Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 20:21 ` Kees Cook
2017-02-11 0:36 ` [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver Luis R. Rodriguez
` (7 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez, Heinrich Schuchardt,
David S. Miller, Ingo Molnar
To keep parity with regular int interfaces provide the an unsigned
int proc_douintvec_minmax() which allows you to specify a range of
allowed valid numbers.
Adding proc_douintvec_minmax_sysadmin() is easy but we can wait for
an actual user for that.
Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
fs/proc/proc_sysctl.c | 4 ++-
include/linux/sysctl.h | 3 +++
kernel/sysctl.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 73696a73a1ec..bc231740239d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1035,7 +1035,8 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
{
int err = 0;
- if (table->proc_handler == proc_douintvec) {
+ if ((table->proc_handler == proc_douintvec) ||
+ (table->proc_handler == proc_douintvec_minmax)) {
if (table->maxlen != sizeof(unsigned int))
err |= sysctl_err(path, table, "array now allowed");
}
@@ -1053,6 +1054,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
if ((table->proc_handler == proc_dostring) ||
(table->proc_handler == proc_dointvec) ||
(table->proc_handler == proc_douintvec) ||
+ (table->proc_handler == proc_douintvec_minmax) ||
(table->proc_handler == proc_dointvec_minmax) ||
(table->proc_handler == proc_dointvec_jiffies) ||
(table->proc_handler == proc_dointvec_userhz_jiffies) ||
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index adf4e51cf597..a35d40ecc211 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -47,6 +47,9 @@ extern int proc_douintvec(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
extern int proc_dointvec_minmax(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
+extern int proc_douintvec_minmax(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos);
extern int proc_dointvec_jiffies(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 493bc05e546a..b286f57e9abe 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2533,6 +2533,65 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
do_proc_dointvec_minmax_conv, ¶m);
}
+struct do_proc_douintvec_minmax_conv_param {
+ unsigned int *min;
+ unsigned int *max;
+};
+
+static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data)
+{
+ struct do_proc_douintvec_minmax_conv_param *param = data;
+
+ if (write) {
+ unsigned int val = *lvalp;
+
+ if ((param->min && *param->min > val) ||
+ (param->max && *param->max < val))
+ return -ERANGE;
+
+ if (*lvalp > UINT_MAX)
+ return -EINVAL;
+ *valp = val;
+ } else {
+ unsigned int val = *valp;
+ *lvalp = (unsigned long) val;
+ }
+
+ return 0;
+}
+
+/**
+ * proc_douintvec_minmax - read a vector of unsigned ints with min/max values
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
+ * values from/to the user buffer, treated as an ASCII string. Negative
+ * strings are not allowed.
+ *
+ * This routine will ensure the values are within the range specified by
+ * table->extra1 (min) and table->extra2 (max). There is a final sanity
+ * check for UINT_MAX to avoid having to support wrap around uses from
+ * userspace.
+ *
+ * Returns 0 on success.
+ */
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ struct do_proc_douintvec_minmax_conv_param param = {
+ .min = (unsigned int *) table->extra1,
+ .max = (unsigned int *) table->extra2,
+ };
+ return do_proc_douintvec(table, write, buffer, lenp, ppos,
+ do_proc_douintvec_minmax_conv, ¶m);
+}
+
static void validate_coredump_safety(void)
{
#ifdef CONFIG_COREDUMP
@@ -3041,6 +3100,12 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
return -ENOSYS;
}
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ return -ENOSYS;
+}
+
int proc_dointvec_jiffies(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
@@ -3083,6 +3148,7 @@ EXPORT_SYMBOL(proc_dointvec);
EXPORT_SYMBOL(proc_douintvec);
EXPORT_SYMBOL(proc_dointvec_jiffies);
EXPORT_SYMBOL(proc_dointvec_minmax);
+EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
EXPORT_SYMBOL(proc_dostring);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 3/9] sysctl: add unsigned int range support
2017-02-11 0:36 ` [PATCH v2 3/9] sysctl: add unsigned int range support Luis R. Rodriguez
@ 2017-02-13 20:21 ` Kees Cook
0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2017-02-13 20:21 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML, Heinrich Schuchardt,
David S. Miller, Ingo Molnar
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> To keep parity with regular int interfaces provide the an unsigned
> int proc_douintvec_minmax() which allows you to specify a range of
> allowed valid numbers.
>
> Adding proc_douintvec_minmax_sysadmin() is easy but we can wait for
> an actual user for that.
>
> Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
Acked-by: Kees Cook <keescook@chromium.org>
This looks sensible. :)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (2 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 3/9] sysctl: add unsigned int range support Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 20:27 ` Kees Cook
2017-02-11 0:36 ` [PATCH v2 5/9] test_sysctl: add generic script to expand on tests Luis R. Rodriguez
` (6 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Although we have had tools/testing/selftests/sysctl/ with two
test cases these use existing kernel sysctl interfaces. We want
to expand test coverage, we can't just be looking for random
safe production values to poke at, instead just dedicate a test
driver for debugging purposes and port the existing scripts to
use it. This will make it easier for further tests to be added.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
lib/Kconfig.debug | 11 +++
lib/Makefile | 1 +
lib/test_sysctl.c | 106 ++++++++++++++++++++++++
tools/testing/selftests/sysctl/config | 1 +
tools/testing/selftests/sysctl/run_numerictests | 4 +-
tools/testing/selftests/sysctl/run_stringtests | 4 +-
6 files changed, 123 insertions(+), 4 deletions(-)
create mode 100644 lib/test_sysctl.c
create mode 100644 tools/testing/selftests/sysctl/config
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 64c03b07ad2f..d753fac41f78 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1975,6 +1975,17 @@ config TEST_FIRMWARE
If unsure, say N.
+config TEST_SYSCTL
+ tristate "sysctl test driver"
+ default n
+ depends on PROC_SYSCTL
+ help
+ This builds the "test_sysctl" module. This driver enables to test the
+ proc sysctl interfaces available to drivers safely without affecting
+ production knobs which might alter system functionality.
+
+ If unsure, say N.
+
config TEST_UDELAY
tristate "udelay test driver"
default n
diff --git a/lib/Makefile b/lib/Makefile
index 445a39c21f46..ac832c440d41 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
obj-y += kstrtox.o
obj-$(CONFIG_TEST_BPF) += test_bpf.o
obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
+obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
obj-$(CONFIG_TEST_KASAN) += test_kasan.o
obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
new file mode 100644
index 000000000000..9b9ae1a95ab3
--- /dev/null
+++ b/lib/test_sysctl.c
@@ -0,0 +1,106 @@
+/*
+ * proc sysctl test driver
+ *
+ * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of copyleft-next (version 0.3.1 or later) as published
+ * at http://copyleft-next.org/.
+ */
+
+/*
+ * This module provides an interface to the the proc sysctl interfaces. This
+ * driver requires CONFIG_PROC_SYSCTL. It will not normally be loaded by the
+ * system unless explicitly requested by name. You can also build this driver
+ * into your kernel.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/async.h>
+#include <linux/delay.h>
+#include <linux/vmalloc.h>
+
+static int i_zero;
+static int i_one_hundred = 100;
+
+struct test_sysctl_data {
+ int int_0001;
+ char string_0001[65];
+};
+
+static struct test_sysctl_data test_data = {
+ .int_0001 = 60,
+ .string_0001 = "(none)",
+};
+
+/* These are all under /proc/sys/debug/test_sysctl/ */
+static struct ctl_table test_table[] = {
+ {
+ .procname = "int_0001",
+ .data = &test_data.int_0001,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &i_zero,
+ .extra2 = &i_one_hundred,
+ },
+ {
+ .procname = "string_0001",
+ .data = &test_data.string_0001,
+ .maxlen = sizeof(test_data.string_0001),
+ .mode = 0644,
+ .proc_handler = proc_dostring,
+ },
+ { }
+};
+
+static struct ctl_table test_sysctl_table[] = {
+ {
+ .procname = "test_sysctl",
+ .maxlen = 0,
+ .mode = 0555,
+ .child = test_table,
+ },
+ { }
+};
+
+static struct ctl_table test_sysctl_root_table[] = {
+ {
+ .procname = "debug",
+ .maxlen = 0,
+ .mode = 0555,
+ .child = test_sysctl_table,
+ },
+ { }
+};
+
+static struct ctl_table_header *test_sysctl_header;
+
+static int __init test_sysctl_init(void)
+{
+ test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
+ if (!test_sysctl_header)
+ return -ENOMEM;
+ return 0;
+}
+late_initcall(test_sysctl_init);
+
+static void __exit test_sysctl_exit(void)
+{
+ if (test_sysctl_header)
+ unregister_sysctl_table(test_sysctl_header);
+}
+
+module_exit(test_sysctl_exit);
+
+MODULE_AUTHOR("Luis R. Rodriguez <mcgrof@kernel.org>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/sysctl/config b/tools/testing/selftests/sysctl/config
new file mode 100644
index 000000000000..6ca14800d755
--- /dev/null
+++ b/tools/testing/selftests/sysctl/config
@@ -0,0 +1 @@
+CONFIG_TEST_SYSCTL=y
diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
index 8510f93f2d14..cdfeef96568c 100755
--- a/tools/testing/selftests/sysctl/run_numerictests
+++ b/tools/testing/selftests/sysctl/run_numerictests
@@ -1,7 +1,7 @@
#!/bin/sh
-SYSCTL="/proc/sys"
-TARGET="${SYSCTL}/vm/swappiness"
+SYSCTL="/proc/sys/debug/test_sysctl/"
+TARGET="${SYSCTL}/int_0001"
ORIG=$(cat "${TARGET}")
TEST_STR=$(( $ORIG + 1 ))
diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
index 90a9293d520c..15a646b2c527 100755
--- a/tools/testing/selftests/sysctl/run_stringtests
+++ b/tools/testing/selftests/sysctl/run_stringtests
@@ -1,7 +1,7 @@
#!/bin/sh
-SYSCTL="/proc/sys"
-TARGET="${SYSCTL}/kernel/domainname"
+SYSCTL="/proc/sys/debug/test_sysctl/"
+TARGET="${SYSCTL}/string_0001"
ORIG=$(cat "${TARGET}")
TEST_STR="Testing sysctl"
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver
2017-02-11 0:36 ` [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver Luis R. Rodriguez
@ 2017-02-13 20:27 ` Kees Cook
0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2017-02-13 20:27 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Although we have had tools/testing/selftests/sysctl/ with two
> test cases these use existing kernel sysctl interfaces. We want
> to expand test coverage, we can't just be looking for random
> safe production values to poke at, instead just dedicate a test
> driver for debugging purposes and port the existing scripts to
> use it. This will make it easier for further tests to be added.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
Yes please!
Acked-by: Kees Cook <keescook@chromium.org>
> ---
> lib/Kconfig.debug | 11 +++
> lib/Makefile | 1 +
> lib/test_sysctl.c | 106 ++++++++++++++++++++++++
> tools/testing/selftests/sysctl/config | 1 +
> tools/testing/selftests/sysctl/run_numerictests | 4 +-
> tools/testing/selftests/sysctl/run_stringtests | 4 +-
> 6 files changed, 123 insertions(+), 4 deletions(-)
> create mode 100644 lib/test_sysctl.c
> create mode 100644 tools/testing/selftests/sysctl/config
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 64c03b07ad2f..d753fac41f78 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1975,6 +1975,17 @@ config TEST_FIRMWARE
>
> If unsure, say N.
>
> +config TEST_SYSCTL
> + tristate "sysctl test driver"
> + default n
> + depends on PROC_SYSCTL
> + help
> + This builds the "test_sysctl" module. This driver enables to test the
> + proc sysctl interfaces available to drivers safely without affecting
> + production knobs which might alter system functionality.
> +
> + If unsure, say N.
> +
> config TEST_UDELAY
> tristate "udelay test driver"
> default n
> diff --git a/lib/Makefile b/lib/Makefile
> index 445a39c21f46..ac832c440d41 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
> obj-y += kstrtox.o
> obj-$(CONFIG_TEST_BPF) += test_bpf.o
> obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
> +obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
> obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
> obj-$(CONFIG_TEST_KASAN) += test_kasan.o
> obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
> diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
> new file mode 100644
> index 000000000000..9b9ae1a95ab3
> --- /dev/null
> +++ b/lib/test_sysctl.c
> @@ -0,0 +1,106 @@
> +/*
> + * proc sysctl test driver
> + *
> + * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of copyleft-next (version 0.3.1 or later) as published
> + * at http://copyleft-next.org/.
> + */
> +
> +/*
> + * This module provides an interface to the the proc sysctl interfaces. This
> + * driver requires CONFIG_PROC_SYSCTL. It will not normally be loaded by the
> + * system unless explicitly requested by name. You can also build this driver
> + * into your kernel.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/async.h>
> +#include <linux/delay.h>
> +#include <linux/vmalloc.h>
> +
> +static int i_zero;
> +static int i_one_hundred = 100;
> +
> +struct test_sysctl_data {
> + int int_0001;
> + char string_0001[65];
I wonder if it's worth adding a comment here that "65" is tied to the
sysctl string self-test.
> +};
> +
> +static struct test_sysctl_data test_data = {
> + .int_0001 = 60,
> + .string_0001 = "(none)",
> +};
> +
> +/* These are all under /proc/sys/debug/test_sysctl/ */
> +static struct ctl_table test_table[] = {
> + {
> + .procname = "int_0001",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + },
> + {
> + .procname = "string_0001",
> + .data = &test_data.string_0001,
> + .maxlen = sizeof(test_data.string_0001),
> + .mode = 0644,
> + .proc_handler = proc_dostring,
> + },
> + { }
> +};
> +
> +static struct ctl_table test_sysctl_table[] = {
> + {
> + .procname = "test_sysctl",
> + .maxlen = 0,
> + .mode = 0555,
> + .child = test_table,
> + },
> + { }
> +};
> +
> +static struct ctl_table test_sysctl_root_table[] = {
> + {
> + .procname = "debug",
> + .maxlen = 0,
> + .mode = 0555,
> + .child = test_sysctl_table,
> + },
> + { }
> +};
> +
> +static struct ctl_table_header *test_sysctl_header;
> +
> +static int __init test_sysctl_init(void)
> +{
> + test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
> + if (!test_sysctl_header)
> + return -ENOMEM;
> + return 0;
> +}
> +late_initcall(test_sysctl_init);
> +
> +static void __exit test_sysctl_exit(void)
> +{
> + if (test_sysctl_header)
> + unregister_sysctl_table(test_sysctl_header);
> +}
> +
> +module_exit(test_sysctl_exit);
> +
> +MODULE_AUTHOR("Luis R. Rodriguez <mcgrof@kernel.org>");
> +MODULE_LICENSE("GPL");
> diff --git a/tools/testing/selftests/sysctl/config b/tools/testing/selftests/sysctl/config
> new file mode 100644
> index 000000000000..6ca14800d755
> --- /dev/null
> +++ b/tools/testing/selftests/sysctl/config
> @@ -0,0 +1 @@
> +CONFIG_TEST_SYSCTL=y
Technically, this works with =m too, but whatever reads "config" would
need to know to load it first...
> diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
> index 8510f93f2d14..cdfeef96568c 100755
> --- a/tools/testing/selftests/sysctl/run_numerictests
> +++ b/tools/testing/selftests/sysctl/run_numerictests
> @@ -1,7 +1,7 @@
> #!/bin/sh
>
> -SYSCTL="/proc/sys"
> -TARGET="${SYSCTL}/vm/swappiness"
> +SYSCTL="/proc/sys/debug/test_sysctl/"
> +TARGET="${SYSCTL}/int_0001"
> ORIG=$(cat "${TARGET}")
> TEST_STR=$(( $ORIG + 1 ))
>
> diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
> index 90a9293d520c..15a646b2c527 100755
> --- a/tools/testing/selftests/sysctl/run_stringtests
> +++ b/tools/testing/selftests/sysctl/run_stringtests
> @@ -1,7 +1,7 @@
> #!/bin/sh
>
> -SYSCTL="/proc/sys"
> -TARGET="${SYSCTL}/kernel/domainname"
> +SYSCTL="/proc/sys/debug/test_sysctl/"
> +TARGET="${SYSCTL}/string_0001"
> ORIG=$(cat "${TARGET}")
> TEST_STR="Testing sysctl"
>
> --
> 2.11.0
>
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 5/9] test_sysctl: add generic script to expand on tests
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (3 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 20:30 ` Kees Cook
2017-02-11 0:36 ` [PATCH v2 6/9] test_sysctl: test against PAGE_SIZE for int Luis R. Rodriguez
` (5 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
This adds a generic script to let us more easily add more tests
cases. Since we really have only two types of tests cases just
fold them into the one file. Each test unit is now identified
into its separate function:
# ./sysctl.sh -l
Test ID list:
TEST_ID x NUM_TEST
TEST_ID: Test ID
NUM_TESTS: Number of recommended times to run the test
0001 x 1 - tests proc_dointvec_minmax()
0002 x 1 - tests proc_dostring()
For now we start off with what we had before, and run only each test once.
We can now watch a test case until it fails:
./sysctl.sh -w 0002
We can also run a test case x number of times, say we want to run
a test case 100 times:
./sysctl.sh -c 0001 100
To run a test case only once, for example:
./sysctl.sh -s 0002
The default settings are specified at the top of sysctl.sh.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
tools/testing/selftests/sysctl/Makefile | 3 +-
tools/testing/selftests/sysctl/common_tests | 109 ------
tools/testing/selftests/sysctl/run_numerictests | 10 -
tools/testing/selftests/sysctl/run_stringtests | 77 ----
tools/testing/selftests/sysctl/sysctl.sh | 459 ++++++++++++++++++++++++
5 files changed, 460 insertions(+), 198 deletions(-)
delete mode 100644 tools/testing/selftests/sysctl/common_tests
delete mode 100755 tools/testing/selftests/sysctl/run_numerictests
delete mode 100755 tools/testing/selftests/sysctl/run_stringtests
create mode 100755 tools/testing/selftests/sysctl/sysctl.sh
diff --git a/tools/testing/selftests/sysctl/Makefile b/tools/testing/selftests/sysctl/Makefile
index b3c33e071f10..95c320b354e8 100644
--- a/tools/testing/selftests/sysctl/Makefile
+++ b/tools/testing/selftests/sysctl/Makefile
@@ -4,8 +4,7 @@
# No binaries, but make sure arg-less "make" doesn't trigger "run_tests".
all:
-TEST_PROGS := run_numerictests run_stringtests
-TEST_FILES := common_tests
+TEST_PROGS := sysctl.sh
include ../lib.mk
diff --git a/tools/testing/selftests/sysctl/common_tests b/tools/testing/selftests/sysctl/common_tests
deleted file mode 100644
index 17d534b1b7b4..000000000000
--- a/tools/testing/selftests/sysctl/common_tests
+++ /dev/null
@@ -1,109 +0,0 @@
-#!/bin/sh
-
-TEST_FILE=$(mktemp)
-
-echo "== Testing sysctl behavior against ${TARGET} =="
-
-set_orig()
-{
- echo "${ORIG}" > "${TARGET}"
-}
-
-set_test()
-{
- echo "${TEST_STR}" > "${TARGET}"
-}
-
-verify()
-{
- local seen
- seen=$(cat "$1")
- if [ "${seen}" != "${TEST_STR}" ]; then
- return 1
- fi
- return 0
-}
-
-trap 'set_orig; rm -f "${TEST_FILE}"' EXIT
-
-rc=0
-
-echo -n "Writing test file ... "
-echo "${TEST_STR}" > "${TEST_FILE}"
-if ! verify "${TEST_FILE}"; then
- echo "FAIL" >&2
- exit 1
-else
- echo "ok"
-fi
-
-echo -n "Checking sysctl is not set to test value ... "
-if verify "${TARGET}"; then
- echo "FAIL" >&2
- exit 1
-else
- echo "ok"
-fi
-
-echo -n "Writing sysctl from shell ... "
-set_test
-if ! verify "${TARGET}"; then
- echo "FAIL" >&2
- exit 1
-else
- echo "ok"
-fi
-
-echo -n "Resetting sysctl to original value ... "
-set_orig
-if verify "${TARGET}"; then
- echo "FAIL" >&2
- exit 1
-else
- echo "ok"
-fi
-
-# Now that we've validated the sanity of "set_test" and "set_orig",
-# we can use those functions to set starting states before running
-# specific behavioral tests.
-
-echo -n "Writing entire sysctl in single write ... "
-set_orig
-dd if="${TEST_FILE}" of="${TARGET}" bs=4096 2>/dev/null
-if ! verify "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Writing middle of sysctl after synchronized seek ... "
-set_test
-dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 skip=1 2>/dev/null
-if ! verify "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Writing beyond end of sysctl ... "
-set_orig
-dd if="${TEST_FILE}" of="${TARGET}" bs=20 seek=2 2>/dev/null
-if verify "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Writing sysctl with multiple long writes ... "
-set_orig
-(perl -e 'print "A" x 50;'; echo "${TEST_STR}") | \
- dd of="${TARGET}" bs=50 2>/dev/null
-if verify "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
deleted file mode 100755
index cdfeef96568c..000000000000
--- a/tools/testing/selftests/sysctl/run_numerictests
+++ /dev/null
@@ -1,10 +0,0 @@
-#!/bin/sh
-
-SYSCTL="/proc/sys/debug/test_sysctl/"
-TARGET="${SYSCTL}/int_0001"
-ORIG=$(cat "${TARGET}")
-TEST_STR=$(( $ORIG + 1 ))
-
-. ./common_tests
-
-exit $rc
diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
deleted file mode 100755
index 15a646b2c527..000000000000
--- a/tools/testing/selftests/sysctl/run_stringtests
+++ /dev/null
@@ -1,77 +0,0 @@
-#!/bin/sh
-
-SYSCTL="/proc/sys/debug/test_sysctl/"
-TARGET="${SYSCTL}/string_0001"
-ORIG=$(cat "${TARGET}")
-TEST_STR="Testing sysctl"
-
-. ./common_tests
-
-# Only string sysctls support seeking/appending.
-MAXLEN=65
-
-echo -n "Writing entire sysctl in short writes ... "
-set_orig
-dd if="${TEST_FILE}" of="${TARGET}" bs=1 2>/dev/null
-if ! verify "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Writing middle of sysctl after unsynchronized seek ... "
-set_test
-dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 2>/dev/null
-if verify "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Checking sysctl maxlen is at least $MAXLEN ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-2), "B";' | \
- dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
-if ! grep -q B "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Checking sysctl keeps original string on overflow append ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
- dd of="${TARGET}" bs=$(( MAXLEN - 1 )) 2>/dev/null
-if grep -q B "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Checking sysctl stays NULL terminated on write ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
- dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
-if grep -q B "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-echo -n "Checking sysctl stays NULL terminated on overwrite ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-1), "BB";' | \
- dd of="${TARGET}" bs=$(( $MAXLEN + 1 )) 2>/dev/null
-if grep -q B "${TARGET}"; then
- echo "FAIL" >&2
- rc=1
-else
- echo "ok"
-fi
-
-exit $rc
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
new file mode 100755
index 000000000000..f8f29092063d
--- /dev/null
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -0,0 +1,459 @@
+#!/bin/bash
+# Copyright (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org>
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of copyleft-next (version 0.3.1 or later) as published
+# at http://copyleft-next.org/.
+
+# This performs a series tests against the proc sysctl interface.
+
+TEST_NAME="sysctl"
+TEST_DRIVER="test_${TEST_NAME}"
+TEST_DIR=$(dirname $0)
+TEST_FILE=$(mktemp)
+
+# This represents
+#
+# TEST_ID:TEST_COUNT:ENABLED
+#
+# TEST_ID: is the test id number
+# TEST_COUNT: number of times we should run the test
+# ENABLED: 1 if enabled, 0 otherwise
+#
+# Once these are enabled please leave them as-is. Write your own test,
+# we have tons of space.
+ALL_TESTS="0001:1:1"
+ALL_TESTS="$ALL_TESTS 0002:1:1"
+
+test_modprobe()
+{
+ if [ ! -d $DIR ]; then
+ echo "$0: $DIR not present" >&2
+ echo "You must have the following enabled in your kernel:" >&2
+ cat $TEST_DIR/config >&2
+ exit 1
+ fi
+}
+
+function allow_user_defaults()
+{
+ if [ -z $DIR ]; then
+ DIR="/sys/module/test_sysctl/"
+ fi
+ if [ -z $DEFAULT_NUM_TESTS ]; then
+ DEFAULT_NUM_TESTS=50
+ fi
+ if [ -z $SYSCTL ]; then
+ SYSCTL="/proc/sys/debug/test_sysctl"
+ fi
+}
+
+test_reqs()
+{
+ uid=$(id -u)
+ if [ $uid -ne 0 ]; then
+ echo $msg must be run as root >&2
+ exit 0
+ fi
+
+ if ! which perl 2> /dev/null > /dev/null; then
+ echo "$0: You need perl installed"
+ exit 1
+ fi
+}
+
+function load_req_mod()
+{
+ trap "test_modprobe" EXIT
+
+ if [ ! -d $DIR ]; then
+ modprobe $TEST_DRIVER
+ if [ $? -ne 0 ]; then
+ exit
+ fi
+ fi
+}
+
+set_orig()
+{
+ if [ ! -z $TARGET ]; then
+ echo "${ORIG}" > "${TARGET}"
+ fi
+}
+
+set_test()
+{
+ echo "${TEST_STR}" > "${TARGET}"
+}
+
+verify()
+{
+ local seen
+ seen=$(cat "$1")
+ if [ "${seen}" != "${TEST_STR}" ]; then
+ return 1
+ fi
+ return 0
+}
+
+test_rc()
+{
+ if [[ $rc != 0 ]]; then
+ echo "Failed test, return value: $rc" >&2
+ exit $rc
+ fi
+}
+
+test_finish()
+{
+ set_orig
+ rm -f "${TEST_FILE}"
+}
+
+run_numerictests()
+{
+ echo "== Testing sysctl behavior against ${TARGET} =="
+
+ rc=0
+
+ echo -n "Writing test file ... "
+ echo "${TEST_STR}" > "${TEST_FILE}"
+ if ! verify "${TEST_FILE}"; then
+ echo "FAIL" >&2
+ exit 1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Checking sysctl is not set to test value ... "
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ exit 1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Writing sysctl from shell ... "
+ set_test
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ exit 1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Resetting sysctl to original value ... "
+ set_orig
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ exit 1
+ else
+ echo "ok"
+ fi
+
+ # Now that we've validated the sanity of "set_test" and "set_orig",
+ # we can use those functions to set starting states before running
+ # specific behavioral tests.
+
+ echo -n "Writing entire sysctl in single write ... "
+ set_orig
+ dd if="${TEST_FILE}" of="${TARGET}" bs=4096 2>/dev/null
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Writing middle of sysctl after synchronized seek ... "
+ set_test
+ dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 skip=1 2>/dev/null
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Writing beyond end of sysctl ... "
+ set_orig
+ dd if="${TEST_FILE}" of="${TARGET}" bs=20 seek=2 2>/dev/null
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Writing sysctl with multiple long writes ... "
+ set_orig
+ (perl -e 'print "A" x 50;'; echo "${TEST_STR}") | \
+ dd of="${TARGET}" bs=50 2>/dev/null
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ test_rc
+}
+
+run_stringtests()
+{
+ echo -n "Writing entire sysctl in short writes ... "
+ set_orig
+ dd if="${TEST_FILE}" of="${TARGET}" bs=1 2>/dev/null
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Writing middle of sysctl after unsynchronized seek ... "
+ set_test
+ dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 2>/dev/null
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Checking sysctl maxlen is at least $MAXLEN ... "
+ set_orig
+ perl -e 'print "A" x ('"${MAXLEN}"'-2), "B";' | \
+ dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
+ if ! grep -q B "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Checking sysctl keeps original string on overflow append ... "
+ set_orig
+ perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
+ dd of="${TARGET}" bs=$(( MAXLEN - 1 )) 2>/dev/null
+ if grep -q B "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Checking sysctl stays NULL terminated on write ... "
+ set_orig
+ perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
+ dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
+ if grep -q B "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ echo -n "Checking sysctl stays NULL terminated on overwrite ... "
+ set_orig
+ perl -e 'print "A" x ('"${MAXLEN}"'-1), "BB";' | \
+ dd of="${TARGET}" bs=$(( $MAXLEN + 1 )) 2>/dev/null
+ if grep -q B "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+
+ test_rc
+}
+
+sysctl_test_0001()
+{
+ TARGET="${SYSCTL}/int_0001"
+ ORIG=$(cat "${TARGET}")
+ TEST_STR=$(( $ORIG + 1 ))
+
+ run_numerictests
+}
+
+sysctl_test_0002()
+{
+ TARGET="${SYSCTL}/string_0001"
+ ORIG=$(cat "${TARGET}")
+ TEST_STR="Testing sysctl"
+ # Only string sysctls support seeking/appending.
+ MAXLEN=65
+
+ run_numerictests
+ run_stringtests
+}
+
+list_tests()
+{
+ echo "Test ID list:"
+ echo
+ echo "TEST_ID x NUM_TEST"
+ echo "TEST_ID: Test ID"
+ echo "NUM_TESTS: Number of recommended times to run the test"
+ echo
+ echo "0001 x $(get_test_count 0001) - tests proc_dointvec_minmax()"
+ echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
+}
+
+test_reqs
+
+usage()
+{
+ NUM_TESTS=$(grep -o ' ' <<<"$ALL_TESTS" | grep -c .)
+ let NUM_TESTS=$NUM_TESTS+1
+ MAX_TEST=$(printf "%04d\n" $NUM_TESTS)
+ echo "Usage: $0 [ -t <4-number-digit> ] | [ -w <4-number-digit> ] |"
+ echo " [ -s <4-number-digit> ] | [ -c <4-number-digit> <test- count>"
+ echo " [ all ] [ -h | --help ] [ -l ]"
+ echo ""
+ echo "Valid tests: 0001-$MAX_TEST"
+ echo ""
+ echo " all Runs all tests (default)"
+ echo " -t Run test ID the number amount of times is recommended"
+ echo " -w Watch test ID run until it runs into an error"
+ echo " -c Run test ID once"
+ echo " -s Run test ID x test-count number of times"
+ echo " -l List all test ID list"
+ echo " -h|--help Help"
+ echo
+ echo "If an error every occurs execution will immediately terminate."
+ echo "If you are adding a new test try using -w <test-ID> first to"
+ echo "make sure the test passes a series of tests."
+ echo
+ echo Example uses:
+ echo
+ echo "$TEST_NAME.sh -- executes all tests"
+ echo "$TEST_NAME.sh -t 0002 -- Executes test ID 0002 number of times is recomended"
+ echo "$TEST_NAME.sh -w 0002 -- Watch test ID 0002 run until an error occurs"
+ echo "$TEST_NAME.sh -s 0002 -- Run test ID 0002 once"
+ echo "$TEST_NAME.sh -c 0002 3 -- Run test ID 0002 three times"
+ echo
+ list_tests
+ exit 1
+}
+
+function test_num()
+{
+ re='^[0-9]+$'
+ if ! [[ $1 =~ $re ]]; then
+ usage
+ fi
+}
+
+function get_test_count()
+{
+ test_num $1
+ TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+ LAST_TWO=${TEST_DATA#*:*}
+ echo ${LAST_TWO%:*}
+}
+
+function get_test_enabled()
+{
+ test_num $1
+ TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+ echo ${TEST_DATA#*:*:}
+}
+
+function run_all_tests()
+{
+ for i in $ALL_TESTS ; do
+ TEST_ID=${i%:*:*}
+ ENABLED=$(get_test_enabled $TEST_ID)
+ TEST_COUNT=$(get_test_count $TEST_ID)
+ if [[ $ENABLED -eq "1" ]]; then
+ test_case $TEST_ID $TEST_COUNT
+ fi
+ done
+}
+
+function watch_log()
+{
+ if [ $# -ne 3 ]; then
+ clear
+ fi
+ date
+ echo "Running test: $2 - run #$1"
+}
+
+function watch_case()
+{
+ i=0
+ while [ 1 ]; do
+
+ if [ $# -eq 1 ]; then
+ test_num $1
+ watch_log $i ${TEST_NAME}_test_$1
+ ${TEST_NAME}_test_$1
+ else
+ watch_log $i all
+ run_all_tests
+ fi
+ let i=$i+1
+ done
+}
+
+function test_case()
+{
+ NUM_TESTS=$DEFAULT_NUM_TESTS
+ if [ $# -eq 2 ]; then
+ NUM_TESTS=$2
+ fi
+
+ i=0
+ while [ $i -lt $NUM_TESTS ]; do
+ test_num $1
+ watch_log $i ${TEST_NAME}_test_$1 noclear
+ RUN_TEST=${TEST_NAME}_test_$1
+ $RUN_TEST
+ let i=$i+1
+ done
+}
+
+function parse_args()
+{
+ if [ $# -eq 0 ]; then
+ run_all_tests
+ else
+ if [[ "$1" = "all" ]]; then
+ run_all_tests
+ elif [[ "$1" = "-w" ]]; then
+ shift
+ watch_case $@
+ elif [[ "$1" = "-t" ]]; then
+ shift
+ test_num $1
+ test_case $1 $(get_test_count $1)
+ elif [[ "$1" = "-c" ]]; then
+ shift
+ test_num $1
+ test_num $2
+ test_case $1 $2
+ elif [[ "$1" = "-s" ]]; then
+ shift
+ test_case $1 1
+ elif [[ "$1" = "-l" ]]; then
+ list_tests
+ elif [[ "$1" = "-h" || "$1" = "--help" ]]; then
+ usage
+ else
+ usage
+ fi
+ fi
+}
+
+test_reqs
+allow_user_defaults
+load_req_mod
+
+trap "test_finish" EXIT
+
+parse_args $@
+
+exit 0
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 5/9] test_sysctl: add generic script to expand on tests
2017-02-11 0:36 ` [PATCH v2 5/9] test_sysctl: add generic script to expand on tests Luis R. Rodriguez
@ 2017-02-13 20:30 ` Kees Cook
2017-05-16 22:55 ` Luis R. Rodriguez
0 siblings, 1 reply; 34+ messages in thread
From: Kees Cook @ 2017-02-13 20:30 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> This adds a generic script to let us more easily add more tests
> cases. Since we really have only two types of tests cases just
> fold them into the one file. Each test unit is now identified
> into its separate function:
>
> # ./sysctl.sh -l
> Test ID list:
>
> TEST_ID x NUM_TEST
> TEST_ID: Test ID
> NUM_TESTS: Number of recommended times to run the test
>
> 0001 x 1 - tests proc_dointvec_minmax()
> 0002 x 1 - tests proc_dostring()
>
> For now we start off with what we had before, and run only each test once.
> We can now watch a test case until it fails:
>
> ./sysctl.sh -w 0002
>
> We can also run a test case x number of times, say we want to run
> a test case 100 times:
>
> ./sysctl.sh -c 0001 100
>
> To run a test case only once, for example:
>
> ./sysctl.sh -s 0002
>
> The default settings are specified at the top of sysctl.sh.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
I'm not a fan of this: it consolidates tests when it's not needed and
creates a test running infrastructure at the wrong level of
abstraction. I'd like to see individual tests that are one-off
runnable. Whatever consumes the tools/testing/selftests/ tree is what
should be doing the -w, -c, etc style options.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 5/9] test_sysctl: add generic script to expand on tests
2017-02-13 20:30 ` Kees Cook
@ 2017-05-16 22:55 ` Luis R. Rodriguez
0 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-16 22:55 UTC (permalink / raw)
To: Kees Cook
Cc: Luis R. Rodriguez, Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Mon, Feb 13, 2017 at 12:30:22PM -0800, Kees Cook wrote:
> On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > This adds a generic script to let us more easily add more tests
> > cases. Since we really have only two types of tests cases just
> > fold them into the one file. Each test unit is now identified
> > into its separate function:
> >
> > # ./sysctl.sh -l
> > Test ID list:
> >
> > TEST_ID x NUM_TEST
> > TEST_ID: Test ID
> > NUM_TESTS: Number of recommended times to run the test
> >
> > 0001 x 1 - tests proc_dointvec_minmax()
> > 0002 x 1 - tests proc_dostring()
> >
> > For now we start off with what we had before, and run only each test once.
> > We can now watch a test case until it fails:
> >
> > ./sysctl.sh -w 0002
> >
> > We can also run a test case x number of times, say we want to run
> > a test case 100 times:
> >
> > ./sysctl.sh -c 0001 100
> >
> > To run a test case only once, for example:
> >
> > ./sysctl.sh -s 0002
> >
> > The default settings are specified at the top of sysctl.sh.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>
> I'm not a fan of this: it consolidates tests when it's not needed
Tests can easily be split off even with the above syntax, right now its all
just stuffed in one file but the syntax does not impede this to change.
> and creates a test running infrastructure at the wrong level of
> abstraction.
In lieu of an existing abstraction layer which provides this having each script
*for now* do what it wishes seems fair. To create an generic test abstraction
layer we need to study each test case, and I'm afraid I can't do that at this
time. But I do by now have about 3 test drivers pending upstream which share
similar testing taste, so I can later try to groom a generic infrastructure for
what I want but for now I can't find the issue with having each test script
have what it needs.
> I'd like to see individual tests that are one-off runnable.
The above allows for this: './sysctl.sh -s 0002' will run the test case 0002
once.
> Whatever consumes the tools/testing/selftests/ tree is what
> should be doing the -w, -c, etc style options.
In the end I agree, but I also believe in evolving this.
If you feel strongly this needs to be generalized at the right layer from the
start I'm afraid I'll just have to drop these tests as I just don't have the
time to address expanding the selftest infrastructure with a generic solution
which covers all that I added, I expect this to be quite a bit of work.
Luis
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 6/9] test_sysctl: test against PAGE_SIZE for int
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (4 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 5/9] test_sysctl: add generic script to expand on tests Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-11 0:36 ` [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case Luis R. Rodriguez
` (4 subsequent siblings)
10 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Add the following tests to ensure we do not regress:
o Test using a buffer full of space (PAGE_SIZE-1) followed by a
single digit works
o Test using a buffer full of spaces (PAGE_SIZE or over) will fail
As tests increase instead of unloading the module and reloading it
we can just do a shell reset_vals() with a reset to values we know
are set at init on the driver.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
tools/testing/selftests/sysctl/sysctl.sh | 65 ++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index f8f29092063d..14b9d875db42 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -46,6 +46,12 @@ function allow_user_defaults()
if [ -z $SYSCTL ]; then
SYSCTL="/proc/sys/debug/test_sysctl"
fi
+ if [ -z $PAGE_SIZE ]; then
+ PAGE_SIZE=$(getconf PAGESIZE)
+ fi
+ if [ -z $MAX_DIGITS ]; then
+ MAX_DIGITS=$(($PAGE_SIZE/8))
+ fi
}
test_reqs()
@@ -60,6 +66,10 @@ test_reqs()
echo "$0: You need perl installed"
exit 1
fi
+ if ! which getconf 2> /dev/null > /dev/null; then
+ echo "$0: You need getconf installed"
+ exit 1
+ fi
}
function load_req_mod()
@@ -74,6 +84,23 @@ function load_req_mod()
fi
}
+reset_vals()
+{
+ VAL=""
+ TRIGGER=$(basename ${TARGET})
+ case "$TRIGGER" in
+ int_0001)
+ VAL="60"
+ ;;
+ string_0001)
+ VAL="(none)"
+ ;;
+ *)
+ ;;
+ esac
+ echo -n $VAL > $TARGET
+}
+
set_orig()
{
if [ ! -z $TARGET ]; then
@@ -195,7 +222,42 @@ run_numerictests()
else
echo "ok"
fi
+ test_rc
+}
+
+# Your test must accept digits 3 and 4 to use this
+run_limit_digit()
+{
+ echo -n "Checking ignoring spaces up to PAGE_SIZE works on write ..."
+ reset_vals
+
+ LIMIT=$((MAX_DIGITS -1))
+ TEST_STR="3"
+ (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+ dd of="${TARGET}" 2>/dev/null
+
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Checking passing PAGE_SIZE of spaces fails on write ..."
+ reset_vals
+ LIMIT=$((MAX_DIGITS))
+ TEST_STR="4"
+ (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+ dd of="${TARGET}" 2>/dev/null
+
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
test_rc
}
@@ -271,15 +333,18 @@ run_stringtests()
sysctl_test_0001()
{
TARGET="${SYSCTL}/int_0001"
+ reset_vals
ORIG=$(cat "${TARGET}")
TEST_STR=$(( $ORIG + 1 ))
run_numerictests
+ run_limit_digit
}
sysctl_test_0002()
{
TARGET="${SYSCTL}/string_0001"
+ reset_vals
ORIG=$(cat "${TARGET}")
TEST_STR="Testing sysctl"
# Only string sysctls support seeking/appending.
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (5 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 6/9] test_sysctl: test against PAGE_SIZE for int Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 22:00 ` Kees Cook
2017-02-11 0:36 ` [PATCH v2 8/9] test_sysctl: add simple proc_douintvec() case Luis R. Rodriguez
` (3 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Test against a simple proc_dointvec() case. While at it, add
a test against INT_MAX. Make sure INT_MAX works, and INT_MAX+1
will fail. Also test negative values work.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
lib/test_sysctl.c | 11 ++++++
tools/testing/selftests/sysctl/sysctl.sh | 62 ++++++++++++++++++++++++++++++++
2 files changed, 73 insertions(+)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 9b9ae1a95ab3..c36a024d7351 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -34,11 +34,15 @@ static int i_one_hundred = 100;
struct test_sysctl_data {
int int_0001;
+ int int_0002;
+
char string_0001[65];
};
static struct test_sysctl_data test_data = {
.int_0001 = 60,
+ .int_0002 = 1,
+
.string_0001 = "(none)",
};
@@ -54,6 +58,13 @@ static struct ctl_table test_table[] = {
.extra2 = &i_one_hundred,
},
{
+ .procname = "int_0002",
+ .data = &test_data.int_0002,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ {
.procname = "string_0001",
.data = &test_data.string_0001,
.maxlen = sizeof(test_data.string_0001),
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 14b9d875db42..45fd2ee5739c 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -24,6 +24,7 @@ TEST_FILE=$(mktemp)
# we have tons of space.
ALL_TESTS="0001:1:1"
ALL_TESTS="$ALL_TESTS 0002:1:1"
+ALL_TESTS="$ALL_TESTS 0003:1:1"
test_modprobe()
{
@@ -52,6 +53,9 @@ function allow_user_defaults()
if [ -z $MAX_DIGITS ]; then
MAX_DIGITS=$(($PAGE_SIZE/8))
fi
+ if [ -z $INT_MAX ]; then
+ INT_MAX=$(getconf INT_MAX)
+ fi
}
test_reqs()
@@ -92,6 +96,9 @@ reset_vals()
int_0001)
VAL="60"
;;
+ int_0002)
+ VAL="1"
+ ;;
string_0001)
VAL="(none)"
;;
@@ -261,6 +268,48 @@ run_limit_digit()
test_rc
}
+# You are using an int
+run_limit_digit_int()
+{
+ echo -n "Testing INT_MAX works ..."
+ reset_vals
+ TEST_STR="$INT_MAX"
+ echo -n $TEST_STR > $TARGET
+
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing INT_MAX + 1 will fail as expected..."
+ reset_vals
+ TEST_STR=$(($INT_MAX+1))
+ echo -n $TEST_STR > $TARGET 2> /dev/null
+
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing negative values will work as expected..."
+ reset_vals
+ TEST_STR="-3"
+ echo -n $TEST_STR > $TARGET 2> /dev/null
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+}
+
run_stringtests()
{
echo -n "Writing entire sysctl in short writes ... "
@@ -354,6 +403,18 @@ sysctl_test_0002()
run_stringtests
}
+sysctl_test_0003()
+{
+ TARGET="${SYSCTL}/int_0002"
+ reset_vals
+ ORIG=$(cat "${TARGET}")
+ TEST_STR=$(( $ORIG + 1 ))
+
+ run_numerictests
+ run_limit_digit
+ run_limit_digit_int
+}
+
list_tests()
{
echo "Test ID list:"
@@ -364,6 +425,7 @@ list_tests()
echo
echo "0001 x $(get_test_count 0001) - tests proc_dointvec_minmax()"
echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
+ echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
}
test_reqs
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case
2017-02-11 0:36 ` [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case Luis R. Rodriguez
@ 2017-02-13 22:00 ` Kees Cook
2017-05-16 22:46 ` Luis R. Rodriguez
0 siblings, 1 reply; 34+ messages in thread
From: Kees Cook @ 2017-02-13 22:00 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Test against a simple proc_dointvec() case. While at it, add
> a test against INT_MAX. Make sure INT_MAX works, and INT_MAX+1
> will fail. Also test negative values work.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> lib/test_sysctl.c | 11 ++++++
> tools/testing/selftests/sysctl/sysctl.sh | 62 ++++++++++++++++++++++++++++++++
> 2 files changed, 73 insertions(+)
>
> diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
> index 9b9ae1a95ab3..c36a024d7351 100644
> --- a/lib/test_sysctl.c
> +++ b/lib/test_sysctl.c
> @@ -34,11 +34,15 @@ static int i_one_hundred = 100;
>
> struct test_sysctl_data {
> int int_0001;
> + int int_0002;
> +
> char string_0001[65];
> };
>
> static struct test_sysctl_data test_data = {
> .int_0001 = 60,
> + .int_0002 = 1,
> +
> .string_0001 = "(none)",
> };
>
> @@ -54,6 +58,13 @@ static struct ctl_table test_table[] = {
> .extra2 = &i_one_hundred,
> },
> {
> + .procname = "int_0002",
> + .data = &test_data.int_0002,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> + {
> .procname = "string_0001",
> .data = &test_data.string_0001,
> .maxlen = sizeof(test_data.string_0001),
> diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
> index 14b9d875db42..45fd2ee5739c 100755
> --- a/tools/testing/selftests/sysctl/sysctl.sh
> +++ b/tools/testing/selftests/sysctl/sysctl.sh
> @@ -24,6 +24,7 @@ TEST_FILE=$(mktemp)
> # we have tons of space.
> ALL_TESTS="0001:1:1"
> ALL_TESTS="$ALL_TESTS 0002:1:1"
> +ALL_TESTS="$ALL_TESTS 0003:1:1"
>
> test_modprobe()
> {
> @@ -52,6 +53,9 @@ function allow_user_defaults()
> if [ -z $MAX_DIGITS ]; then
> MAX_DIGITS=$(($PAGE_SIZE/8))
> fi
> + if [ -z $INT_MAX ]; then
> + INT_MAX=$(getconf INT_MAX)
> + fi
> }
>
> test_reqs()
> @@ -92,6 +96,9 @@ reset_vals()
> int_0001)
> VAL="60"
> ;;
> + int_0002)
> + VAL="1"
> + ;;
> string_0001)
> VAL="(none)"
> ;;
> @@ -261,6 +268,48 @@ run_limit_digit()
> test_rc
> }
>
> +# You are using an int
> +run_limit_digit_int()
> +{
> + echo -n "Testing INT_MAX works ..."
> + reset_vals
> + TEST_STR="$INT_MAX"
> + echo -n $TEST_STR > $TARGET
> +
> + if ! verify "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing INT_MAX + 1 will fail as expected..."
> + reset_vals
> + TEST_STR=$(($INT_MAX+1))
Is the shell always going to do the right thing here? Maybe these test
values should be explicitly hard-coded? I'm on the fence...
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case
2017-02-13 22:00 ` Kees Cook
@ 2017-05-16 22:46 ` Luis R. Rodriguez
0 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-16 22:46 UTC (permalink / raw)
To: Kees Cook
Cc: Luis R. Rodriguez, Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Mon, Feb 13, 2017 at 02:00:13PM -0800, Kees Cook wrote:
> On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
> > index 14b9d875db42..45fd2ee5739c 100755
> > --- a/tools/testing/selftests/sysctl/sysctl.sh
> > +++ b/tools/testing/selftests/sysctl/sysctl.sh
> > @@ -24,6 +24,7 @@ TEST_FILE=$(mktemp)
> > @@ -261,6 +268,48 @@ run_limit_digit()
> > test_rc
> > }
> >
> > +# You are using an int
> > +run_limit_digit_int()
> > +{
> > + echo -n "Testing INT_MAX works ..."
> > + reset_vals
> > + TEST_STR="$INT_MAX"
> > + echo -n $TEST_STR > $TARGET
> > +
> > + if ! verify "${TARGET}"; then
> > + echo "FAIL" >&2
> > + rc=1
> > + else
> > + echo "ok"
> > + fi
> > + test_rc
> > +
> > + echo -n "Testing INT_MAX + 1 will fail as expected..."
> > + reset_vals
> > + TEST_STR=$(($INT_MAX+1))
>
> Is the shell always going to do the right thing here? Maybe these test
> values should be explicitly hard-coded? I'm on the fence...
Will use the good 'ol time tested:
let TEST_STR=$INT_MAX+1
I had used it for all other sums before, not sure why I went short-cut mode.
Either way this is requiring /bin/bash at the top header, but yeah not sure
when that short cut mode addition was added to bash. Better to be both safe
and consistent.
Luis
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 8/9] test_sysctl: add simple proc_douintvec() case
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (6 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-11 0:36 ` [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support Luis R. Rodriguez
` (2 subsequent siblings)
10 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Test against a simple proc_douintvec() case. While at it, add
a test against UINT_MAX. Make sure UINT_MAX works, and UINT_MAX+1
will fail and that negative values are not accepted.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
lib/test_sysctl.c | 11 ++++++
tools/testing/selftests/sysctl/sysctl.sh | 63 ++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index c36a024d7351..1654f41961b7 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -36,6 +36,8 @@ struct test_sysctl_data {
int int_0001;
int int_0002;
+ unsigned int uint_0001;
+
char string_0001[65];
};
@@ -43,6 +45,8 @@ static struct test_sysctl_data test_data = {
.int_0001 = 60,
.int_0002 = 1,
+ .uint_0001 = 314,
+
.string_0001 = "(none)",
};
@@ -65,6 +69,13 @@ static struct ctl_table test_table[] = {
.proc_handler = proc_dointvec,
},
{
+ .procname = "uint_0001",
+ .data = &test_data.uint_0001,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_douintvec,
+ },
+ {
.procname = "string_0001",
.data = &test_data.string_0001,
.maxlen = sizeof(test_data.string_0001),
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 45fd2ee5739c..eedfba6f0a57 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -25,6 +25,7 @@ TEST_FILE=$(mktemp)
ALL_TESTS="0001:1:1"
ALL_TESTS="$ALL_TESTS 0002:1:1"
ALL_TESTS="$ALL_TESTS 0003:1:1"
+ALL_TESTS="$ALL_TESTS 0004:1:1"
test_modprobe()
{
@@ -56,6 +57,9 @@ function allow_user_defaults()
if [ -z $INT_MAX ]; then
INT_MAX=$(getconf INT_MAX)
fi
+ if [ -z $UINT_MAX ]; then
+ UINT_MAX=$(getconf UINT_MAX)
+ fi
}
test_reqs()
@@ -99,6 +103,9 @@ reset_vals()
int_0002)
VAL="1"
;;
+ uint_0001)
+ VAL="314"
+ ;;
string_0001)
VAL="(none)"
;;
@@ -310,6 +317,49 @@ run_limit_digit_int()
test_rc
}
+# You are using an unsigned int
+run_limit_digit_uint()
+{
+ echo -n "Testing UINT_MAX works ..."
+ reset_vals
+ TEST_STR="$UINT_MAX"
+ echo -n $TEST_STR > $TARGET
+
+ if ! verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing UINT_MAX + 1 will fail as expected..."
+ reset_vals
+ TEST_STR=$(($UINT_MAX+1))
+ echo -n $TEST_STR > $TARGET 2> /dev/null
+
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing negative values will not work as expected ..."
+ reset_vals
+ TEST_STR="-3"
+ echo -n $TEST_STR > $TARGET 2> /dev/null
+
+ if verify "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+}
+
run_stringtests()
{
echo -n "Writing entire sysctl in short writes ... "
@@ -415,6 +465,18 @@ sysctl_test_0003()
run_limit_digit_int
}
+sysctl_test_0004()
+{
+ TARGET="${SYSCTL}/uint_0001"
+ reset_vals
+ ORIG=$(cat "${TARGET}")
+ TEST_STR=$(( $ORIG + 1 ))
+
+ run_numerictests
+ run_limit_digit
+ run_limit_digit_uint
+}
+
list_tests()
{
echo "Test ID list:"
@@ -426,6 +488,7 @@ list_tests()
echo "0001 x $(get_test_count 0001) - tests proc_dointvec_minmax()"
echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
+ echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
}
test_reqs
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (7 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 8/9] test_sysctl: add simple proc_douintvec() case Luis R. Rodriguez
@ 2017-02-11 0:36 ` Luis R. Rodriguez
2017-02-13 22:07 ` Kees Cook
2017-02-13 20:11 ` [PATCH v2 0/9] sysctl: add and fix proper unsigned int support Kees Cook
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
10 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-02-11 0:36 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
dmitry.torokhov, shuah, torvalds, linux, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Add a few initial respective tests for an array:
o Echoing values separated by spaces works
o Echoing only first elements will set first elements
o Confirm PAGE_SIZE limit still applies even if an array is used
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
lib/test_sysctl.c | 13 +++++
tools/testing/selftests/sysctl/sysctl.sh | 89 ++++++++++++++++++++++++++++++++
2 files changed, 102 insertions(+)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 1654f41961b7..603c24b2f6cb 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -35,6 +35,7 @@ static int i_one_hundred = 100;
struct test_sysctl_data {
int int_0001;
int int_0002;
+ int int_0003[4];
unsigned int uint_0001;
@@ -45,6 +46,11 @@ static struct test_sysctl_data test_data = {
.int_0001 = 60,
.int_0002 = 1,
+ .int_0003[0] = 0,
+ .int_0003[1] = 1,
+ .int_0003[2] = 2,
+ .int_0003[3] = 3,
+
.uint_0001 = 314,
.string_0001 = "(none)",
@@ -69,6 +75,13 @@ static struct ctl_table test_table[] = {
.proc_handler = proc_dointvec,
},
{
+ .procname = "int_0003",
+ .data = &test_data.int_0003,
+ .maxlen = sizeof(test_data.int_0003),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ {
.procname = "uint_0001",
.data = &test_data.uint_0001,
.maxlen = sizeof(unsigned int),
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index eedfba6f0a57..963d572155b1 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -26,6 +26,7 @@ ALL_TESTS="0001:1:1"
ALL_TESTS="$ALL_TESTS 0002:1:1"
ALL_TESTS="$ALL_TESTS 0003:1:1"
ALL_TESTS="$ALL_TESTS 0004:1:1"
+ALL_TESTS="$ALL_TESTS 0005:3:1"
test_modprobe()
{
@@ -78,6 +79,10 @@ test_reqs()
echo "$0: You need getconf installed"
exit 1
fi
+ if ! which diff 2> /dev/null > /dev/null; then
+ echo "$0: You need diff installed"
+ exit 1
+ fi
}
function load_req_mod()
@@ -137,6 +142,12 @@ verify()
return 0
}
+verify_diff_w()
+{
+ echo "$TEST_STR" | diff -w -u - $1 2>&1 > /dev/null
+ return $?
+}
+
test_rc()
{
if [[ $rc != 0 ]]; then
@@ -317,6 +328,74 @@ run_limit_digit_int()
test_rc
}
+# You used an int array
+run_limit_digit_int_array()
+{
+ echo -n "Testing array works as expected ... "
+ TEST_STR="4 3 2 1"
+ echo -n $TEST_STR > $TARGET
+
+ if ! verify_diff_w "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing skipping trailing array elements works ... "
+ # Do not reset_vals, carry on the values from the last test.
+ # If we only echo in two digits the last two are left intact
+ TEST_STR="100 101"
+ echo -n $TEST_STR > $TARGET
+ # After we echo in, to help diff we need to set on TEST_STR what
+ # we expect the result to be.
+ TEST_STR="100 101 2 1"
+
+ if ! verify_diff_w "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing PAGE_SIZE limit on array works ... "
+ # Do not reset_vals, carry on the values from the last test.
+ # Even if you use an int array, you are still restricted to
+ # MAX_DIGITS, this is a known limitation. Test limit works.
+ LIMIT=$((MAX_DIGITS -1))
+ TEST_STR="9"
+ (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+ dd of="${TARGET}" 2>/dev/null
+
+ TEST_STR="9 101 2 1"
+ if ! verify_diff_w "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+
+ echo -n "Testing exceeding PAGE_SIZE limit fails as expected ... "
+ # Do not reset_vals, carry on the values from the last test.
+ # Now go over limit.
+ LIMIT=$((MAX_DIGITS))
+ TEST_STR="7"
+ (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+ dd of="${TARGET}" 2>/dev/null
+
+ TEST_STR="7 101 2 1"
+ if verify_diff_w "${TARGET}"; then
+ echo "FAIL" >&2
+ rc=1
+ else
+ echo "ok"
+ fi
+ test_rc
+}
+
# You are using an unsigned int
run_limit_digit_uint()
{
@@ -477,6 +556,15 @@ sysctl_test_0004()
run_limit_digit_uint
}
+sysctl_test_0005()
+{
+ TARGET="${SYSCTL}/int_0003"
+ reset_vals
+ ORIG=$(cat "${TARGET}")
+
+ run_limit_digit_int_array
+}
+
list_tests()
{
echo "Test ID list:"
@@ -489,6 +577,7 @@ list_tests()
echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
+ echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array"
}
test_reqs
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support
2017-02-11 0:36 ` [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support Luis R. Rodriguez
@ 2017-02-13 22:07 ` Kees Cook
2017-05-16 22:40 ` Luis R. Rodriguez
0 siblings, 1 reply; 34+ messages in thread
From: Kees Cook @ 2017-02-13 22:07 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, Deepa Dinamani, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Add a few initial respective tests for an array:
>
> o Echoing values separated by spaces works
> o Echoing only first elements will set first elements
> o Confirm PAGE_SIZE limit still applies even if an array is used
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> lib/test_sysctl.c | 13 +++++
> tools/testing/selftests/sysctl/sysctl.sh | 89 ++++++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+)
>
> diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
> index 1654f41961b7..603c24b2f6cb 100644
> --- a/lib/test_sysctl.c
> +++ b/lib/test_sysctl.c
> @@ -35,6 +35,7 @@ static int i_one_hundred = 100;
> struct test_sysctl_data {
> int int_0001;
> int int_0002;
> + int int_0003[4];
>
> unsigned int uint_0001;
>
> @@ -45,6 +46,11 @@ static struct test_sysctl_data test_data = {
> .int_0001 = 60,
> .int_0002 = 1,
>
> + .int_0003[0] = 0,
> + .int_0003[1] = 1,
> + .int_0003[2] = 2,
> + .int_0003[3] = 3,
> +
> .uint_0001 = 314,
>
> .string_0001 = "(none)",
> @@ -69,6 +75,13 @@ static struct ctl_table test_table[] = {
> .proc_handler = proc_dointvec,
> },
> {
> + .procname = "int_0003",
> + .data = &test_data.int_0003,
> + .maxlen = sizeof(test_data.int_0003),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> + {
> .procname = "uint_0001",
> .data = &test_data.uint_0001,
> .maxlen = sizeof(unsigned int),
> diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
> index eedfba6f0a57..963d572155b1 100755
> --- a/tools/testing/selftests/sysctl/sysctl.sh
> +++ b/tools/testing/selftests/sysctl/sysctl.sh
> @@ -26,6 +26,7 @@ ALL_TESTS="0001:1:1"
> ALL_TESTS="$ALL_TESTS 0002:1:1"
> ALL_TESTS="$ALL_TESTS 0003:1:1"
> ALL_TESTS="$ALL_TESTS 0004:1:1"
> +ALL_TESTS="$ALL_TESTS 0005:3:1"
>
> test_modprobe()
> {
> @@ -78,6 +79,10 @@ test_reqs()
> echo "$0: You need getconf installed"
> exit 1
> fi
> + if ! which diff 2> /dev/null > /dev/null; then
> + echo "$0: You need diff installed"
> + exit 1
> + fi
> }
>
> function load_req_mod()
> @@ -137,6 +142,12 @@ verify()
> return 0
> }
>
> +verify_diff_w()
> +{
> + echo "$TEST_STR" | diff -w -u - $1 2>&1 > /dev/null
Instead of shell redirection, just use -q ?
> + return $?
> +}
> +
> test_rc()
> {
> if [[ $rc != 0 ]]; then
> @@ -317,6 +328,74 @@ run_limit_digit_int()
> test_rc
> }
>
> +# You used an int array
> +run_limit_digit_int_array()
> +{
> + echo -n "Testing array works as expected ... "
> + TEST_STR="4 3 2 1"
> + echo -n $TEST_STR > $TARGET
> +
> + if ! verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing skipping trailing array elements works ... "
> + # Do not reset_vals, carry on the values from the last test.
> + # If we only echo in two digits the last two are left intact
> + TEST_STR="100 101"
> + echo -n $TEST_STR > $TARGET
> + # After we echo in, to help diff we need to set on TEST_STR what
> + # we expect the result to be.
> + TEST_STR="100 101 2 1"
> +
> + if ! verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing PAGE_SIZE limit on array works ... "
> + # Do not reset_vals, carry on the values from the last test.
> + # Even if you use an int array, you are still restricted to
> + # MAX_DIGITS, this is a known limitation. Test limit works.
> + LIMIT=$((MAX_DIGITS -1))
> + TEST_STR="9"
> + (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
> + dd of="${TARGET}" 2>/dev/null
> +
> + TEST_STR="9 101 2 1"
> + if ! verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing exceeding PAGE_SIZE limit fails as expected ... "
> + # Do not reset_vals, carry on the values from the last test.
> + # Now go over limit.
> + LIMIT=$((MAX_DIGITS))
> + TEST_STR="7"
> + (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
> + dd of="${TARGET}" 2>/dev/null
> +
> + TEST_STR="7 101 2 1"
> + if verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +}
> +
> # You are using an unsigned int
> run_limit_digit_uint()
> {
> @@ -477,6 +556,15 @@ sysctl_test_0004()
> run_limit_digit_uint
> }
>
> +sysctl_test_0005()
> +{
> + TARGET="${SYSCTL}/int_0003"
> + reset_vals
> + ORIG=$(cat "${TARGET}")
> +
> + run_limit_digit_int_array
> +}
> +
> list_tests()
> {
> echo "Test ID list:"
> @@ -489,6 +577,7 @@ list_tests()
> echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
> echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
> echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
> + echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array"
> }
>
> test_reqs
> --
> 2.11.0
>
I love seeing these tests added. I have one other change I'd like to
add to sysctl, but I haven't had time to make sure it doesn't break
stuff. I haven't been able to prove it to myself, but I think it's
safe; I just need to update the tests to handle it:
http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=sysctl/writes_strict&id=b63a38ca45bd9fb61545ce6ce66093147eb96a7c
It'd need an update for the uint handler...
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support
2017-02-13 22:07 ` Kees Cook
@ 2017-05-16 22:40 ` Luis R. Rodriguez
0 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-16 22:40 UTC (permalink / raw)
To: Kees Cook
Cc: Luis R. Rodriguez, Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, Deepa Dinamani, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Mon, Feb 13, 2017 at 02:07:53PM -0800, Kees Cook wrote:
> On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
> > index eedfba6f0a57..963d572155b1 100755
> > --- a/tools/testing/selftests/sysctl/sysctl.sh
> > +++ b/tools/testing/selftests/sysctl/sysctl.sh
> > @@ -137,6 +142,12 @@ verify()
> > return 0
> > }
> >
> > +verify_diff_w()
> > +{
> > + echo "$TEST_STR" | diff -w -u - $1 2>&1 > /dev/null
>
> Instead of shell redirection, just use -q ?
Will try.
> I love seeing these tests added. I have one other change I'd like to
> add to sysctl,
Upon a glance again at this stuff I can think of a few other checks
but one battle at a time...
> but I haven't had time to make sure it doesn't break
> stuff. I haven't been able to prove it to myself, but I think it's
> safe; I just need to update the tests to handle it:
>
> http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=sysctl/writes_strict&id=b63a38ca45bd9fb61545ce6ce66093147eb96a7c
>
> It'd need an update for the uint handler...
That would also expands on the definition of the strict mode. I think this is
fair if we take it for granted strict will always aim for correctness, but we
also have to be fair and be clear on possible impact and ensure nothing will
bust. I have a feeling though that we'd keep on going with these semantics on
and on and on... which really is just irritating and it tells me something more
wrong about this crap interface.
Just a rant here...
Luis
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 0/9] sysctl: add and fix proper unsigned int support
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (8 preceding siblings ...)
2017-02-11 0:36 ` [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support Luis R. Rodriguez
@ 2017-02-13 20:11 ` Kees Cook
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
10 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2017-02-13 20:11 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Al Viro, Andrew Morton, Eric W. Biederman,
Arnaldo Carvalho de Melo, Ingo Molnar, Mel Gorman,
Subash Abhinov Kasiviswanathan, Jessica Yu, Rusty Russell,
Steven Whitehouse, deepa.kernel, Matt Fleming, Alexey Dobriyan,
Borislav Petkov, Dmitry Torokhov, shuah, Linus Torvalds,
Guenter Roeck, linux-kselftest, LKML
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On this v2 I've taken Alexey's recommendation and looked at array users
> of the proc sysctl interface which complicate the interfece to see if
> we can instead just simplify the unsigned int implementation. I could
> not find any clear candidate. As such I've just ripped out array
> support.
>
> Since some future unsigned int proc sysctl users might think there is
> array support I've taken measures to do sanity checks on initialization
> and warn the kernel if such users creep up. To validate this I ended up
> just writing a simple test driver, and extending our tests. In doing this
> I also found a really old issue with sysctl_check_table(), and yet another
> issue with the first incarnation of proc_douintvec().
>
> I hammered on proc_douintvec() as much as I could, and extended tests for
> this to ensure we don't regress should some int users convert over.
>
> I noticed one more issue but I did not fix as I figured it was worth
> discussing: proc_doi*_minmax() handlers have historically allowed users
> to register even if their own data does not match the expressed min/max
> values. When this happens the value is exposed on /proc/sys but reading
> or writing does not work against it. I'm of the opinion that
> sysctl_check_table() should just validate this and bail preventing such
> entries from ever creeping up. The only reason I didn't do this is this
> *could* mean some tables don't get registered in some cases -- I haven't
> done the vetting. If we're fine with this I can add it later.
>
> Luis R. Rodriguez (9):
> sysctl: fix lax sysctl_check_table() sanity check
> sysctl: add proper unsigned int support
> sysctl: add unsigned int range support
> test_sysctl: add dedicated proc sysctl test driver
> test_sysctl: add generic script to expand on tests
> test_sysctl: test against PAGE_SIZE for int
> test_sysctl: add simple proc_dointvec() case
> test_sysctl: add simple proc_douintvec() case
> test_sysctl: test against int proc_dointvec() array support
Please go ahead and add a MAINTAINERS file entry for the two of us
(and Eric if he wants) for sysctl. We poke at it enough that really we
should declare it maintained (as you suggested privately). For now we
should likely still land it all through akpm, though.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH v3 0/5] sysctl: few fixes
2017-02-11 0:36 ` [PATCH v2 0/9] sysctl: add and fix " Luis R. Rodriguez
` (9 preceding siblings ...)
2017-02-13 20:11 ` [PATCH v2 0/9] sysctl: add and fix proper unsigned int support Kees Cook
@ 2017-05-19 3:35 ` Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
` (4 more replies)
10 siblings, 5 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-19 3:35 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
zlpnobody, dmitry.torokhov, shuah, torvalds, linux, linux-kernel,
Luis R. Rodriguez
I've been working on making kmod more deterministic, and as I did that
I couldn't help but notice a few issues with sysctl. My end goal was just
to fix unsigned int support, which back then was completely broken. Liping
Zhang has sent up small atomic fixes, however it still missed yet one more
fix and Alexey Dobriyan had also suggested to just drop array support given
its complexity.
I have inspected array support using Coccinelle and indeed its not that
popular, so if in fact we can avoid it for new interfaces, I agree its best.
I did develop a sysctl stress driver but will hold that off for another series.
Luis R. Rodriguez (5):
sysctl: fix lax sysctl_check_table() sanity check
sysctl: kdoc'ify sysctl_writes_strict
sysctl: fold sysctl_writes_strict checks into helper
sysctl: simplify unsigned int support
sysctl: add unsigned int range support
fs/proc/proc_sysctl.c | 26 ++++-
include/linux/sysctl.h | 3 +
kernel/sysctl.c | 304 +++++++++++++++++++++++++++++++++++++++++++------
3 files changed, 293 insertions(+), 40 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
@ 2017-05-19 3:35 ` Luis R. Rodriguez
2017-05-22 22:40 ` Andrew Morton
2017-05-19 3:35 ` [PATCH v3 2/5] sysctl: kdoc'ify sysctl_writes_strict Luis R. Rodriguez
` (3 subsequent siblings)
4 siblings, 1 reply; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-19 3:35 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
zlpnobody, dmitry.torokhov, shuah, torvalds, linux, linux-kernel,
Luis R. Rodriguez
Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
improved sanity checks considerbly, however the enhancements on
sysctl_check_table() meant adding a functional change so that
only the last table entry's sanity error is propagated. It also
changed the way errors were propagated so that each new check
reset the err value, this means only last sanity check computed
is used for an error. This has been in the kernel since v3.4 days.
Fix this by carrying on errors from previous checks and iterations
as we traverse the table and ensuring we keep any error from previous
checks. We keep iterating on the table even if an error is found so
we can complain for all errors found in one shot. This works as
-EINVAL is always returned on error anyway, and the check for error
is any non-zero value.
Fixes: 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
fs/proc/proc_sysctl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 67985a7233c2..32c9c5630507 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1066,7 +1066,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
int err = 0;
for (; table->procname; table++) {
if (table->child)
- err = sysctl_err(path, table, "Not a file");
+ err |= sysctl_err(path, table, "Not a file");
if ((table->proc_handler == proc_dostring) ||
(table->proc_handler == proc_dointvec) ||
@@ -1078,15 +1078,15 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
(table->proc_handler == proc_doulongvec_minmax) ||
(table->proc_handler == proc_doulongvec_ms_jiffies_minmax)) {
if (!table->data)
- err = sysctl_err(path, table, "No data");
+ err |= sysctl_err(path, table, "No data");
if (!table->maxlen)
- err = sysctl_err(path, table, "No maxlen");
+ err |= sysctl_err(path, table, "No maxlen");
}
if (!table->proc_handler)
- err = sysctl_err(path, table, "No proc_handler");
+ err |= sysctl_err(path, table, "No proc_handler");
if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
- err = sysctl_err(path, table, "bogus .mode 0%o",
+ err |= sysctl_err(path, table, "bogus .mode 0%o",
table->mode);
}
return err;
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check
2017-05-19 3:35 ` [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
@ 2017-05-22 22:40 ` Andrew Morton
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Morton @ 2017-05-22 22:40 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: viro, ebiederm, keescook, acme, mingo, mgorman, subashab, jeyu,
rusty, swhiteho, deepa.kernel, matt, adobriyan, bp, zlpnobody,
dmitry.torokhov, shuah, torvalds, linux, linux-kernel
On Thu, 18 May 2017 20:35:50 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
> improved sanity checks considerbly, however the enhancements on
> sysctl_check_table() meant adding a functional change so that
> only the last table entry's sanity error is propagated. It also
> changed the way errors were propagated so that each new check
> reset the err value, this means only last sanity check computed
> is used for an error. This has been in the kernel since v3.4 days.
>
> Fix this by carrying on errors from previous checks and iterations
> as we traverse the table and ensuring we keep any error from previous
> checks. We keep iterating on the table even if an error is found so
> we can complain for all errors found in one shot. This works as
> -EINVAL is always returned on error anyway, and the check for error
> is any non-zero value.
>
> ...
>
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1066,7 +1066,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
> int err = 0;
> for (; table->procname; table++) {
> if (table->child)
> - err = sysctl_err(path, table, "Not a file");
> + err |= sysctl_err(path, table, "Not a file");
>
> if ((table->proc_handler == proc_dostring) ||
> (table->proc_handler == proc_dointvec) ||
> @@ -1078,15 +1078,15 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
> (table->proc_handler == proc_doulongvec_minmax) ||
> (table->proc_handler == proc_doulongvec_ms_jiffies_minmax)) {
> if (!table->data)
> - err = sysctl_err(path, table, "No data");
> + err |= sysctl_err(path, table, "No data");
> if (!table->maxlen)
> - err = sysctl_err(path, table, "No maxlen");
> + err |= sysctl_err(path, table, "No maxlen");
> }
> if (!table->proc_handler)
> - err = sysctl_err(path, table, "No proc_handler");
> + err |= sysctl_err(path, table, "No proc_handler");
>
> if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
> - err = sysctl_err(path, table, "bogus .mode 0%o",
> + err |= sysctl_err(path, table, "bogus .mode 0%o",
> table->mode);
> }
> return err;
glumpf. I'm not a fan of this err|=foo() trick - if foo() returns
different errnos we can a mangled result. This patch assumes that
syscal_err() will only ever return a single errno.
I guess we're safe enough in this case but still... ugh.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 2/5] sysctl: kdoc'ify sysctl_writes_strict
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
@ 2017-05-19 3:35 ` Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 3/5] sysctl: fold sysctl_writes_strict checks into helper Luis R. Rodriguez
` (2 subsequent siblings)
4 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-19 3:35 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
zlpnobody, dmitry.torokhov, shuah, torvalds, linux, linux-kernel,
Luis R. Rodriguez
Document the different sysctl_writes_strict modes in code.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
kernel/sysctl.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4dfba1a76cc3..02725178694a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -174,11 +174,32 @@ extern int no_unaligned_warning;
#ifdef CONFIG_PROC_SYSCTL
-#define SYSCTL_WRITES_LEGACY -1
-#define SYSCTL_WRITES_WARN 0
-#define SYSCTL_WRITES_STRICT 1
+/**
+ * enum sysctl_writes_mode - supported sysctl write modes
+ *
+ * @SYSCTL_WRITES_LEGACY: each write syscall must fully contain the sysctl value
+ * to be written, and multiple writes on the same sysctl file descriptor
+ * will rewrite the sysctl value, regardless of file position. No warning
+ * is issued when the initial position is not 0.
+ * @SYSCTL_WRITES_WARN: same as above but warn when the initial file position is
+ * not 0.
+ * @SYSCTL_WRITES_STRICT: writes to numeric sysctl entries must always be at
+ * file position 0 and the value must be fully contained in the buffer
+ * sent to the write syscall. If dealing with strings respect the file
+ * position, but restrict this to the max length of the buffer, anything
+ * passed the max lenght will be ignored. Multiple writes will append
+ * to the buffer.
+ *
+ * These write modes control how current file position affects the behavior of
+ * updating sysctl values through the proc interface on each write.
+ */
+enum sysctl_writes_mode {
+ SYSCTL_WRITES_LEGACY = -1,
+ SYSCTL_WRITES_WARN = 0,
+ SYSCTL_WRITES_STRICT = 1,
+};
-static int sysctl_writes_strict = SYSCTL_WRITES_STRICT;
+static enum sysctl_writes_mode sysctl_writes_strict = SYSCTL_WRITES_STRICT;
static int proc_do_cad_pid(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 3/5] sysctl: fold sysctl_writes_strict checks into helper
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 2/5] sysctl: kdoc'ify sysctl_writes_strict Luis R. Rodriguez
@ 2017-05-19 3:35 ` Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 4/5] sysctl: simplify unsigned int support Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 5/5] sysctl: add unsigned int range support Luis R. Rodriguez
4 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-19 3:35 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
zlpnobody, dmitry.torokhov, shuah, torvalds, linux, linux-kernel,
Luis R. Rodriguez
The mode sysctl_writes_strict positional checks keep being
copy and pasted as we add new proc handlers. Just add a helper
to avoid code duplication.
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
kernel/sysctl.c | 56 ++++++++++++++++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 24 deletions(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 02725178694a..6f3bb1f099fa 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1971,6 +1971,32 @@ static void warn_sysctl_write(struct ctl_table *table)
}
/**
+ * proc_first_pos_non_zero_ignore - check if firs position is allowed
+ * @ppos: file position
+ * @table: the sysctl table
+ *
+ * Returns true if the first position is non-zero and the sysctl_writes_strict
+ * mode indicates this is not allowed for numeric input types. String proc
+ * hadlers can ignore the return value.
+ */
+static bool proc_first_pos_non_zero_ignore(loff_t *ppos,
+ struct ctl_table *table)
+{
+ if (!*ppos)
+ return false;
+
+ switch (sysctl_writes_strict) {
+ case SYSCTL_WRITES_STRICT:
+ return true;
+ case SYSCTL_WRITES_WARN:
+ warn_sysctl_write(table);
+ return false;
+ default:
+ return false;
+ }
+}
+
+/**
* proc_dostring - read a string sysctl
* @table: the sysctl table
* @write: %TRUE if this is a write to the sysctl file
@@ -1990,8 +2016,8 @@ static void warn_sysctl_write(struct ctl_table *table)
int proc_dostring(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- if (write && *ppos && sysctl_writes_strict == SYSCTL_WRITES_WARN)
- warn_sysctl_write(table);
+ if (write)
+ proc_first_pos_non_zero_ignore(ppos, table);
return _proc_do_string((char *)(table->data), table->maxlen, write,
(char __user *)buffer, lenp, ppos);
@@ -2193,17 +2219,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
conv = do_proc_dointvec_conv;
if (write) {
- if (*ppos) {
- switch (sysctl_writes_strict) {
- case SYSCTL_WRITES_STRICT:
- goto out;
- case SYSCTL_WRITES_WARN:
- warn_sysctl_write(table);
- break;
- default:
- break;
- }
- }
+ if (proc_first_pos_non_zero_ignore(ppos, table))
+ goto out;
if (left > PAGE_SIZE - 1)
left = PAGE_SIZE - 1;
@@ -2468,17 +2485,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
left = *lenp;
if (write) {
- if (*ppos) {
- switch (sysctl_writes_strict) {
- case SYSCTL_WRITES_STRICT:
- goto out;
- case SYSCTL_WRITES_WARN:
- warn_sysctl_write(table);
- break;
- default:
- break;
- }
- }
+ if (proc_first_pos_non_zero_ignore(ppos, table))
+ goto out;
if (left > PAGE_SIZE - 1)
left = PAGE_SIZE - 1;
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 4/5] sysctl: simplify unsigned int support
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
` (2 preceding siblings ...)
2017-05-19 3:35 ` [PATCH v3 3/5] sysctl: fold sysctl_writes_strict checks into helper Luis R. Rodriguez
@ 2017-05-19 3:35 ` Luis R. Rodriguez
2017-05-19 3:35 ` [PATCH v3 5/5] sysctl: add unsigned int range support Luis R. Rodriguez
4 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-19 3:35 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
zlpnobody, dmitry.torokhov, shuah, torvalds, linux, linux-kernel,
Luis R. Rodriguez, Heinrich Schuchardt, David S. Miller,
Ingo Molnar
Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32
fields") added proc_douintvec() to start help adding support for
unsigned int, this however was only half the work needed. Two fixes
have come in since then for the following issues:
o Printing the values shows a negative value, this happens since
do_proc_dointvec() and this uses proc_put_long()
This was fixed by commit 5380e5644afbba9 ("sysctl: don't print negative
flag for proc_douintvec").
o We can easily wrap around the int values: UINT_MAX is 4294967295,
if we echo in 4294967295 + 1 we end up with 0, using 4294967295 + 2
we end up with 1.
o We echo negative values in and they are accepted
This was fixed by commit 425fffd886bae3d1 ("sysctl: report EINVAL if
value is larger than UINT_MAX for proc_douintvec").
It still also failed to be added to sysctl_check_table()... instead
of adding it with the current implementation just provide a proper
and simplified unsigned int support without any array unsigned int
support with no negative support at all.
Historically sysctl proc helpers have supported arrays, due to the
complexity this adds though we've taken a step back to evaluate array
users to determine if its worth upkeeping for unsigned int. An
evaluation using Coccinelle has been done to perform a grammatical
search to ask ourselves:
o How many sysctl proc_dointvec() (int) users exist which likely
should be moved over to proc_douintvec() (unsigned int) ?
Answer: about 8
- Of these how many are array users ?
Answer: Probably only 1
o How many sysctl array users exist ?
Answer: about 12
This last question gives us an idea just how popular arrays: they
are not. Array support should probably just be kept for strings.
The identified uint ports are:
drivers/infiniband/core/ucma.c - max_backlog
drivers/infiniband/core/iwcm.c - default_backlog
net/core/sysctl_net_core.c - rps_sock_flow_sysctl()
net/netfilter/nf_conntrack_timestamp.c - nf_conntrack_timestamp -- bool
net/netfilter/nf_conntrack_acct.c nf_conntrack_acct -- bool
net/netfilter/nf_conntrack_ecache.c - nf_conntrack_events -- bool
net/netfilter/nf_conntrack_helper.c - nf_conntrack_helper -- bool
net/phonet/sysctl.c proc_local_port_range()
The only possible array users is proc_local_port_range() but it does not
seem worth it to add array support just for this given the range support
works just as well. Unsigned int support should be desirable more for
when you *need* more than INT_MAX or using int min/max support then
does not suffice for your ranges.
If you forget and by mistake happen to register an unsigned int proc entry
with an array, the driver will fail and you will get something as follows:
sysctl table check failed: debug/test_sysctl//uint_0002 array now allowed
CPU: 2 PID: 1342 Comm: modprobe Tainted: G W E <etc>
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <etc>
Call Trace:
dump_stack+0x63/0x81
__register_sysctl_table+0x350/0x650
? kmem_cache_alloc_trace+0x107/0x240
__register_sysctl_paths+0x1b3/0x1e0
? 0xffffffffc005f000
register_sysctl_table+0x1f/0x30
test_sysctl_init+0x10/0x1000 [test_sysctl]
do_one_initcall+0x52/0x1a0
? kmem_cache_alloc_trace+0x107/0x240
do_init_module+0x5f/0x200
load_module+0x1867/0x1bd0
? __symbol_put+0x60/0x60
SYSC_finit_module+0xdf/0x110
SyS_finit_module+0xe/0x10
entry_SYSCALL_64_fastpath+0x1e/0xad
RIP: 0033:0x7f042b22d119
<etc>
Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Liping Zhang <zlpnobody@gmail.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
fs/proc/proc_sysctl.c | 14 +++++
kernel/sysctl.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 160 insertions(+), 7 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 32c9c5630507..ee6feba8b6c0 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1061,6 +1061,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
return -EINVAL;
}
+static int sysctl_check_table_array(const char *path, struct ctl_table *table)
+{
+ int err = 0;
+
+ if (table->proc_handler == proc_douintvec) {
+ if (table->maxlen != sizeof(unsigned int))
+ err |= sysctl_err(path, table, "array now allowed");
+ }
+
+ return err;
+}
+
static int sysctl_check_table(const char *path, struct ctl_table *table)
{
int err = 0;
@@ -1081,6 +1093,8 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
err |= sysctl_err(path, table, "No data");
if (!table->maxlen)
err |= sysctl_err(path, table, "No maxlen");
+ else
+ err |= sysctl_check_table_array(path, table);
}
if (!table->proc_handler)
err |= sysctl_err(path, table, "No proc_handler");
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6f3bb1f099fa..d12078fc215f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2175,19 +2175,18 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
return 0;
}
-static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
- int *valp,
- int write, void *data)
+static int do_proc_douintvec_conv(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data)
{
if (write) {
- if (*negp)
+ if (*lvalp > UINT_MAX)
return -EINVAL;
if (*lvalp > UINT_MAX)
return -EINVAL;
*valp = *lvalp;
} else {
unsigned int val = *valp;
- *negp = false;
*lvalp = (unsigned long)val;
}
return 0;
@@ -2287,6 +2286,146 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
buffer, lenp, ppos, conv, data);
}
+static int do_proc_douintvec_w(unsigned int *tbl_data,
+ struct ctl_table *table,
+ void __user *buffer,
+ size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ unsigned long lval;
+ int err = 0;
+ size_t left;
+ bool neg;
+ char *kbuf = NULL, *p;
+
+ left = *lenp;
+
+ if (proc_first_pos_non_zero_ignore(ppos, table))
+ goto bail_early;
+
+ if (left > PAGE_SIZE - 1)
+ left = PAGE_SIZE - 1;
+
+ p = kbuf = memdup_user_nul(buffer, left);
+ if (IS_ERR(kbuf))
+ return -EINVAL;
+
+ left -= proc_skip_spaces(&p);
+ if (!left) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
+ err = proc_get_long(&p, &left, &lval, &neg,
+ proc_wspace_sep,
+ sizeof(proc_wspace_sep), NULL);
+ if (err || neg) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
+ if (conv(&lval, tbl_data, 1, data)) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
+ if (!err && left)
+ left -= proc_skip_spaces(&p);
+
+out_free:
+ kfree(kbuf);
+ if (err)
+ return -EINVAL;
+
+ return 0;
+
+ /* This is in keeping with old __do_proc_dointvec() */
+bail_early:
+ *ppos += *lenp;
+ return err;
+}
+
+static int do_proc_douintvec_r(unsigned int *tbl_data, void __user *buffer,
+ size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ unsigned long lval;
+ int err = 0;
+ size_t left;
+
+ left = *lenp;
+
+ if (conv(&lval, tbl_data, 0, data)) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ err = proc_put_long(&buffer, &left, lval, false);
+ if (err || !left)
+ goto out;
+
+ err = proc_put_char(&buffer, &left, '\n');
+
+out:
+ *lenp -= left;
+ *ppos += *lenp;
+
+ return err;
+}
+
+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
+ int write, void __user *buffer,
+ size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ unsigned int *i, vleft;
+
+ if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
+ *lenp = 0;
+ return 0;
+ }
+
+ i = (unsigned int *) tbl_data;
+ vleft = table->maxlen / sizeof(*i);
+
+ /*
+ * Arrays are not supported, keep this simple. *Do not* add
+ * support for them.
+ */
+ if (vleft != 1) {
+ *lenp = 0;
+ return -EINVAL;
+ }
+
+ if (!conv)
+ conv = do_proc_douintvec_conv;
+
+ if (write)
+ return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
+ conv, data);
+ return do_proc_douintvec_r(i, buffer, lenp, ppos, conv, data);
+}
+
+static int do_proc_douintvec(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos,
+ int (*conv)(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data),
+ void *data)
+{
+ return __do_proc_douintvec(table->data, table, write,
+ buffer, lenp, ppos, conv, data);
+}
+
/**
* proc_dointvec - read a vector of integers
* @table: the sysctl table
@@ -2322,8 +2461,8 @@ int proc_dointvec(struct ctl_table *table, int write,
int proc_douintvec(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- return do_proc_dointvec(table, write, buffer, lenp, ppos,
- do_proc_douintvec_conv, NULL);
+ return do_proc_douintvec(table, write, buffer, lenp, ppos,
+ do_proc_douintvec_conv, NULL);
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 5/5] sysctl: add unsigned int range support
2017-05-19 3:35 ` [PATCH v3 0/5] sysctl: few fixes Luis R. Rodriguez
` (3 preceding siblings ...)
2017-05-19 3:35 ` [PATCH v3 4/5] sysctl: simplify unsigned int support Luis R. Rodriguez
@ 2017-05-19 3:35 ` Luis R. Rodriguez
4 siblings, 0 replies; 34+ messages in thread
From: Luis R. Rodriguez @ 2017-05-19 3:35 UTC (permalink / raw)
To: viro, akpm, ebiederm, keescook, acme, mingo, mgorman, subashab
Cc: jeyu, rusty, swhiteho, deepa.kernel, matt, adobriyan, bp,
zlpnobody, dmitry.torokhov, shuah, torvalds, linux, linux-kernel,
Luis R. Rodriguez, Heinrich Schuchardt, David S. Miller,
Ingo Molnar
To keep parity with regular int interfaces provide the an unsigned
int proc_douintvec_minmax() which allows you to specify a range of
allowed valid numbers.
Adding proc_douintvec_minmax_sysadmin() is easy but we can wait for
an actual user for that.
Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
fs/proc/proc_sysctl.c | 4 ++-
include/linux/sysctl.h | 3 +++
kernel/sysctl.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index ee6feba8b6c0..8f9d564d0969 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1065,7 +1065,8 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
{
int err = 0;
- if (table->proc_handler == proc_douintvec) {
+ if ((table->proc_handler == proc_douintvec) ||
+ (table->proc_handler == proc_douintvec_minmax)) {
if (table->maxlen != sizeof(unsigned int))
err |= sysctl_err(path, table, "array now allowed");
}
@@ -1083,6 +1084,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
if ((table->proc_handler == proc_dostring) ||
(table->proc_handler == proc_dointvec) ||
(table->proc_handler == proc_douintvec) ||
+ (table->proc_handler == proc_douintvec_minmax) ||
(table->proc_handler == proc_dointvec_minmax) ||
(table->proc_handler == proc_dointvec_jiffies) ||
(table->proc_handler == proc_dointvec_userhz_jiffies) ||
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 80d07816def0..225001d437ae 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -47,6 +47,9 @@ extern int proc_douintvec(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
extern int proc_dointvec_minmax(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
+extern int proc_douintvec_minmax(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos);
extern int proc_dointvec_jiffies(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index d12078fc215f..df9f2a367882 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2567,6 +2567,65 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
do_proc_dointvec_minmax_conv, ¶m);
}
+struct do_proc_douintvec_minmax_conv_param {
+ unsigned int *min;
+ unsigned int *max;
+};
+
+static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
+ unsigned int *valp,
+ int write, void *data)
+{
+ struct do_proc_douintvec_minmax_conv_param *param = data;
+
+ if (write) {
+ unsigned int val = *lvalp;
+
+ if ((param->min && *param->min > val) ||
+ (param->max && *param->max < val))
+ return -ERANGE;
+
+ if (*lvalp > UINT_MAX)
+ return -EINVAL;
+ *valp = val;
+ } else {
+ unsigned int val = *valp;
+ *lvalp = (unsigned long) val;
+ }
+
+ return 0;
+}
+
+/**
+ * proc_douintvec_minmax - read a vector of unsigned ints with min/max values
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
+ * values from/to the user buffer, treated as an ASCII string. Negative
+ * strings are not allowed.
+ *
+ * This routine will ensure the values are within the range specified by
+ * table->extra1 (min) and table->extra2 (max). There is a final sanity
+ * check for UINT_MAX to avoid having to support wrap around uses from
+ * userspace.
+ *
+ * Returns 0 on success.
+ */
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ struct do_proc_douintvec_minmax_conv_param param = {
+ .min = (unsigned int *) table->extra1,
+ .max = (unsigned int *) table->extra2,
+ };
+ return do_proc_douintvec(table, write, buffer, lenp, ppos,
+ do_proc_douintvec_minmax_conv, ¶m);
+}
+
static void validate_coredump_safety(void)
{
#ifdef CONFIG_COREDUMP
@@ -3066,6 +3125,12 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
return -ENOSYS;
}
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ return -ENOSYS;
+}
+
int proc_dointvec_jiffies(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
@@ -3108,6 +3173,7 @@ EXPORT_SYMBOL(proc_dointvec);
EXPORT_SYMBOL(proc_douintvec);
EXPORT_SYMBOL(proc_dointvec_jiffies);
EXPORT_SYMBOL(proc_dointvec_minmax);
+EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
EXPORT_SYMBOL(proc_dostring);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread