This issue was discussed at [0] and following, and the solution was to
clamp the size at KMALLOC_MAX_LEN. However, KMALLOC_MAX_LEN is a maximum
allocation, and may be difficult to allocate in low memory conditions.
Since maxlen is already exposed, we can allocate approximately the right
amount directly, fixing up those drivers which set a bogus maxlen. These
drivers were located based on those which had copy_x_user replaced in
32927393dc1c, on the basis that other drivers either use builtin proc_*
handlers, or do not access the data pointer. The latter is OK because
maxlen only needs to be an upper limit.
[0] https://lore.kernel.org/lkml/[email protected]/
Fixes: 32927393dc1c ("sysctl: pass kernel pointers to ->proc_handler")
Signed-off-by: Alex Xu (Hello71) <[email protected]>
---
drivers/parport/procfs.c | 20 ++++++++++----------
fs/proc/proc_sysctl.c | 13 ++++++++-----
include/linux/sysctl.h | 2 +-
net/core/sysctl_net_core.c | 1 +
net/decnet/sysctl_net_decnet.c | 4 ++--
net/sunrpc/xprtrdma/svc_rdma.c | 18 +++++++++---------
6 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index d740eba3c099..a2eeae73f9fa 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -280,28 +280,28 @@ static const struct parport_sysctl_table parport_sysctl_template = {
{
.procname = "base-addr",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 20,
.mode = 0444,
.proc_handler = do_hardware_base_addr
},
{
.procname = "irq",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 20,
.mode = 0444,
.proc_handler = do_hardware_irq
},
{
.procname = "dma",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 20,
.mode = 0444,
.proc_handler = do_hardware_dma
},
{
.procname = "modes",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 40,
.mode = 0444,
.proc_handler = do_hardware_modes
},
@@ -310,35 +310,35 @@ static const struct parport_sysctl_table parport_sysctl_template = {
{
.procname = "autoprobe",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 256,
.mode = 0444,
.proc_handler = do_autoprobe
},
{
.procname = "autoprobe0",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 256,
.mode = 0444,
.proc_handler = do_autoprobe
},
{
.procname = "autoprobe1",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 256,
.mode = 0444,
.proc_handler = do_autoprobe
},
{
.procname = "autoprobe2",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 256,
.mode = 0444,
.proc_handler = do_autoprobe
},
{
.procname = "autoprobe3",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 256,
.mode = 0444,
.proc_handler = do_autoprobe
},
@@ -349,7 +349,7 @@ static const struct parport_sysctl_table parport_sysctl_template = {
{
.procname = "active",
.data = NULL,
- .maxlen = 0,
+ .maxlen = 256,
.mode = 0444,
.proc_handler = do_active_device
},
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index d2018f70d1fa..4a54d3cc174b 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -547,7 +547,7 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
struct inode *inode = file_inode(iocb->ki_filp);
struct ctl_table_header *head = grab_header(inode);
struct ctl_table *table = PROC_I(inode)->sysctl_entry;
- size_t count = iov_iter_count(iter);
+ size_t count = min(table->maxlen, iov_iter_count(iter));
char *kbuf;
ssize_t error;
@@ -567,10 +567,6 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
if (!table->proc_handler)
goto out;
- /* don't even try if the size is too large */
- error = -ENOMEM;
- if (count >= KMALLOC_MAX_SIZE)
- goto out;
kbuf = kzalloc(count + 1, GFP_KERNEL);
if (!kbuf)
goto out;
@@ -1138,6 +1134,13 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
err |= sysctl_err(path, table, "bogus .mode 0%o",
table->mode);
+
+ if (table->maxlen >= KMALLOC_MAX_SIZE)
+ err |= sysctl_err(path, table, "maxlen %ld too big",
+ table->maxlen);
+
+ if ((table->mode & S_IRUGO) && !table->maxlen)
+ err |= sysctl_err(path, table, "cannot read maxlen=0");
}
return err;
}
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 51298a4f4623..78a1d36767f9 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -112,7 +112,7 @@ static inline void *proc_sys_poll_event(struct ctl_table_poll *poll)
struct ctl_table {
const char *procname; /* Text ID for /proc/sys, or zero */
void *data;
- int maxlen;
+ size_t maxlen;
umode_t mode;
struct ctl_table *child; /* Deprecated */
proc_handler *proc_handler; /* Callback for text formatting */
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index d86d8d11cfe4..c51a2e7e0dfb 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -470,6 +470,7 @@ static struct ctl_table net_core_table[] = {
#ifdef CONFIG_NET_FLOW_LIMIT
{
.procname = "flow_limit_cpu_bitmap",
+ .maxlen = 128,
.mode = 0644,
.proc_handler = flow_limit_cpu_sysctl
},
diff --git a/net/decnet/sysctl_net_decnet.c b/net/decnet/sysctl_net_decnet.c
index 67b5ab2657b7..2ca2ac42c40c 100644
--- a/net/decnet/sysctl_net_decnet.c
+++ b/net/decnet/sysctl_net_decnet.c
@@ -239,14 +239,14 @@ static int dn_def_dev_handler(struct ctl_table *table, int write,
static struct ctl_table dn_table[] = {
{
.procname = "node_address",
- .maxlen = 7,
+ .maxlen = DN_ASCBUF_LEN,
.mode = 0644,
.proc_handler = dn_node_address_handler,
},
{
.procname = "node_name",
.data = node_name,
- .maxlen = 7,
+ .maxlen = sizeof(node_name),
.mode = 0644,
.proc_handler = proc_dostring,
},
diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c
index 526da5d4710b..f326ba6825f2 100644
--- a/net/sunrpc/xprtrdma/svc_rdma.c
+++ b/net/sunrpc/xprtrdma/svc_rdma.c
@@ -143,63 +143,63 @@ static struct ctl_table svcrdma_parm_table[] = {
{
.procname = "rdma_stat_read",
.data = &rdma_stat_read,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_recv",
.data = &rdma_stat_recv,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_write",
.data = &rdma_stat_write,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_sq_starve",
.data = &rdma_stat_sq_starve,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_rq_starve",
.data = &rdma_stat_rq_starve,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_rq_poll",
.data = &rdma_stat_rq_poll,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_rq_prod",
.data = &rdma_stat_rq_prod,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_sq_poll",
.data = &rdma_stat_sq_poll,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
{
.procname = "rdma_stat_sq_prod",
.data = &rdma_stat_sq_prod,
- .maxlen = sizeof(atomic_t),
+ .maxlen = 32,
.mode = 0644,
.proc_handler = read_reset_stat,
},
--
2.30.1
Excerpts from Alex Xu (Hello71)'s message of February 15, 2021 9:53 am:
> This issue was discussed at [0] and following, and the solution was to
> clamp the size at KMALLOC_MAX_LEN. However, KMALLOC_MAX_LEN is a maximum
> allocation, and may be difficult to allocate in low memory conditions.
>
> Since maxlen is already exposed, we can allocate approximately the right
> amount directly, fixing up those drivers which set a bogus maxlen. These
> drivers were located based on those which had copy_x_user replaced in
> 32927393dc1c, on the basis that other drivers either use builtin proc_*
> handlers, or do not access the data pointer. The latter is OK because
> maxlen only needs to be an upper limit.
>
> [0] https://lore.kernel.org/lkml/[email protected]/
>
> Fixes: 32927393dc1c ("sysctl: pass kernel pointers to ->proc_handler")
> Signed-off-by: Alex Xu (Hello71) <[email protected]>
Yeah, no, this doesn't work. A bunch of functions call proc_* but don't
set maxlen, and it's annoying to check this statically. Also causes
weird failures elsewhere. May need to think of a better solution here
(kvzalloc?).
On Mon, Feb 15, 2021 at 09:53:05AM -0500, Alex Xu (Hello71) wrote:
> Since maxlen is already exposed, we can allocate approximately the right
> amount directly, fixing up those drivers which set a bogus maxlen. These
> drivers were located based on those which had copy_x_user replaced in
> 32927393dc1c, on the basis that other drivers either use builtin proc_*
> handlers, or do not access the data pointer. The latter is OK because
> maxlen only needs to be an upper limit.
Please split this into one patch each each subsystem that sets maxlen
to 0 and the actual change to proc_sysctl.c.
How do these maxlen = 0 entries even survive the sysctl_check_table
check?
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 459b3085f2dd2870cd53bfdafa6591963a5a1f21 ("proc_sysctl: clamp sizes using table->maxlen")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git Alex-Xu-Hello71/proc_sysctl-clamp-sizes-using-table-maxlen/20210215-225648
in testcase: stress-ng
version: stress-ng-x86_64-0.11-06_20210105
with following parameters:
nr_threads: 10%
disk: 1HDD
testtime: 60s
fs: ext4
class: vm
test: shm-sysv
cpufreq_governor: performance
ucode: 0x5003003
on test machine: 96 threads Intel(R) Xeon(R) Gold 6252 CPU @ 2.10GHz with 512G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 3.591556] AppArmor: AppArmor initialized
[ 3.613545] Dentry cache hash table entries: 16777216 (order: 15, 134217728 bytes, vmalloc)
[ 3.625219] Inode-cache hash table entries: 8388608 (order: 14, 67108864 bytes, vmalloc)
[ 3.626727] Mount-cache hash table entries: 262144 (order: 9, 2097152 bytes, vmalloc)
[ 3.627838] Mountpoint-cache hash table entries: 262144 (order: 9, 2097152 bytes, vmalloc)
[ 3.629702] sysctl table check failed: vm//stat_refresh cannot read maxlen=0
[ 3.630514] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.11.0-rc5-00038-g459b3085f2dd #1
[ 3.631513] Hardware name: Intel Corporation S2600WFT/S2600WFT, BIOS SE5C620.86B.02.01.0008.031920191559 03/19/2019
[ 3.631513] Call Trace:
[ 3.631513] dump_stack (kbuild/src/consumer/lib/dump_stack.c:122)
[ 3.631513] __register_sysctl_table (kbuild/src/consumer/fs/proc/proc_sysctl.c:1366)
[ 3.631513] ? register_leaf_sysctl_tables (kbuild/src/consumer/fs/proc/proc_sysctl.c:1457)
[ 3.631513] register_leaf_sysctl_tables (kbuild/src/consumer/fs/proc/proc_sysctl.c:1457)
[ 3.631513] register_leaf_sysctl_tables (kbuild/src/consumer/fs/proc/proc_sysctl.c:1482)
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml
bin/lkp run compatible-job.yaml
Thanks,
Oliver Sang
Excerpts from Christoph Hellwig's message of February 16, 2021 3:47 am:
> How do these maxlen = 0 entries even survive the sysctl_check_table
> check?
maxlen!=0 is only checked for "default" handlers, e.g. proc_dostring,
proc_dointvec. it is not checked for non-default handlers, because some
of them use fixed lengths.
my patch is not correct though because some drivers neither set proper
maxlen nor use memcpy themselves; instead, they construct a ctl_table on
the stack and call proc_*.
> Please split this into one patch each each subsystem that sets maxlen
> to 0 and the actual change to proc_sysctl.c.
I will do this with a new patch version once I figure out a way to
comprehensively fix all the drivers setting bogus values for maxlen
(sometimes maxlen=0 is valid if only blank writes are permitted, and
some drivers set random values which have no relation to the actual read
size).
Thank you for the review.