2021-11-26 14:39:41

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v1 0/2] ucounts: Fix rlimit max values check

Checking the rlimit value specified in init_user_ns from the created userns does
not work properly. The issue is that the maximum value is taken by the same
rules as for ucounts. Because of this, we check the current rlimit counter value
with RLIM_INFINITY in init_user_ns.

--

Alexey Gladkov (2):
ucounts: Fix rlimit max values check
ucounts: Move rlimit max values from ucounts max

include/linux/user_namespace.h | 13 ++++++++++---
kernel/fork.c | 8 ++++----
kernel/ucount.c | 15 +++++++++------
kernel/user_namespace.c | 8 ++++----
4 files changed, 27 insertions(+), 17 deletions(-)

--
2.33.0



2021-11-26 14:39:43

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v1 2/2] ucounts: Move rlimit max values from ucounts max

Since the semantics of maximum rlimit values are different, they need to
be separated from ucount_max. This will prevent the error of using
inc_count/dec_ucount for rlimit parameters.

This patch also renames the functions to emphasize the lack of
connection between rlimit_max and ucount_max.

Signed-off-by: Alexey Gladkov <[email protected]>
---
include/linux/user_namespace.h | 13 ++++++++++---
kernel/fork.c | 8 ++++----
kernel/ucount.c | 6 +++---
kernel/user_namespace.c | 8 ++++----
4 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 33a4240e6a6f..47fd841dec43 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -62,6 +62,7 @@ enum ucount_type {
};

#define MAX_PER_NAMESPACE_UCOUNTS UCOUNT_RLIMIT_NPROC
+#define RLIMIT_COUNTS UCOUNT_COUNTS - MAX_PER_NAMESPACE_UCOUNTS + 1

struct user_namespace {
struct uid_gid_map uid_map;
@@ -98,7 +99,8 @@ struct user_namespace {
struct ctl_table_header *sysctls;
#endif
struct ucounts *ucounts;
- long ucount_max[UCOUNT_COUNTS];
+ long ucount_max[MAX_PER_NAMESPACE_UCOUNTS];
+ long rlimit_max[RLIMIT_COUNTS];
} __randomize_layout;

struct ucounts {
@@ -131,10 +133,15 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type);
void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type);
bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max);

-static inline void set_rlimit_ucount_max(struct user_namespace *ns,
+static inline long get_userns_rlimit_max(struct user_namespace *ns, enum ucount_type type)
+{
+ return READ_ONCE(ns->rlimit_max[type - MAX_PER_NAMESPACE_UCOUNTS]);
+}
+
+static inline void set_userns_rlimit_max(struct user_namespace *ns,
enum ucount_type type, unsigned long max)
{
- ns->ucount_max[type] = max <= LONG_MAX ? max : LONG_MAX;
+ ns->rlimit_max[type - MAX_PER_NAMESPACE_UCOUNTS] = max <= LONG_MAX ? max : LONG_MAX;
}

#ifdef CONFIG_USER_NS
diff --git a/kernel/fork.c b/kernel/fork.c
index 3244cc56b697..e4fce0303a26 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -839,10 +839,10 @@ void __init fork_init(void)
for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++)
init_user_ns.ucount_max[i] = max_threads/2;

- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);

#ifdef CONFIG_VMAP_STACK
cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "fork:vm_stack_cache",
diff --git a/kernel/ucount.c b/kernel/ucount.c
index 7b32c356ebc5..426bd22b0d6d 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -273,7 +273,7 @@ long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
ret = LONG_MAX;
else if (iter == ucounts)
ret = new;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
}
return ret;
}
@@ -322,7 +322,7 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
goto unwind;
if (iter == ucounts)
ret = new;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
/*
* Grab an extra ucount reference for the caller when
* the rlimit count was previously 0.
@@ -350,7 +350,7 @@ bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsign
for (iter = ucounts; iter; iter = iter->ns->ucounts) {
if (get_ucounts_value(iter, type) > max)
return true;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
}
return false;
}
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 6b2e3ca7ee99..106ad0a6188c 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -122,10 +122,10 @@ int create_user_ns(struct cred *new)
for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++) {
ns->ucount_max[i] = INT_MAX;
}
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
ns->ucounts = ucounts;

/* Inherit USERNS_SETGROUPS_ALLOWED from our parent */
--
2.33.0


2021-11-26 14:39:44

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v1 1/2] ucounts: Fix rlimit max values check

The semantics of the rlimit max values differs from ucounts itself. When
creating a new userns, we store the current rlimit of the process in
ucount_max. Thus, the value of the limit in the parent userns is saved
in the created one.

The problem is that now we are taking the maximum value for counter from
the same userns. So for init_user_ns it will always be RLIM_INFINITY.

To fix the problem we need to check the counter value with the max value
stored in userns.

Reproducer:

su - test -c "ulimit -u 3; sleep 5 & sleep 6 & unshare -U --map-root-user sh -c 'sleep 7 & sleep 8 & date; wait'"

Before:

[1] 175
[2] 176
Fri Nov 26 13:48:20 UTC 2021
[1]- Done sleep 5
[2]+ Done sleep 6

After:

[1] 167
[2] 168
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: Interrupted system call
[1]- Done sleep 5
[2]+ Done sleep 6

Fixes: c54b245d0118 ("Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace")
Reported-by: Gleb Fotengauer-Malinovskiy <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
Signed-off-by: Alexey Gladkov <[email protected]>
---
kernel/ucount.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/ucount.c b/kernel/ucount.c
index 4f5613dac227..7b32c356ebc5 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -264,15 +264,16 @@ void dec_ucount(struct ucounts *ucounts, enum ucount_type type)
long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
{
struct ucounts *iter;
+ long max = LONG_MAX;
long ret = 0;

for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- long max = READ_ONCE(iter->ns->ucount_max[type]);
long new = atomic_long_add_return(v, &iter->ucount[type]);
if (new < 0 || new > max)
ret = LONG_MAX;
else if (iter == ucounts)
ret = new;
+ max = READ_ONCE(iter->ns->ucount_max[type]);
}
return ret;
}
@@ -312,15 +313,16 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
{
/* Caller must hold a reference to ucounts */
struct ucounts *iter;
+ long max = LONG_MAX;
long dec, ret = 0;

for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- long max = READ_ONCE(iter->ns->ucount_max[type]);
long new = atomic_long_add_return(1, &iter->ucount[type]);
if (new < 0 || new > max)
goto unwind;
if (iter == ucounts)
ret = new;
+ max = READ_ONCE(iter->ns->ucount_max[type]);
/*
* Grab an extra ucount reference for the caller when
* the rlimit count was previously 0.
@@ -339,15 +341,16 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
return 0;
}

-bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max)
+bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long rlimit)
{
struct ucounts *iter;
- if (get_ucounts_value(ucounts, type) > max)
- return true;
+ long max = rlimit;
+ if (rlimit > LONG_MAX)
+ max = LONG_MAX;
for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- max = READ_ONCE(iter->ns->ucount_max[type]);
if (get_ucounts_value(iter, type) > max)
return true;
+ max = READ_ONCE(iter->ns->ucount_max[type]);
}
return false;
}
--
2.33.0


2021-11-29 07:49:41

by Oliver Sang

[permalink] [raw]
Subject: [ucounts] dc7e5f9d41: UBSAN:array-index-out-of-bounds_in_kernel/ucount.c



Greeting,

FYI, we noticed the following commit (built with gcc-9):

commit: dc7e5f9d419cb31b7751e87cf576f23a0153147c ("[PATCH v1 2/2] ucounts: Move rlimit max values from ucounts max")
url: https://github.com/0day-ci/linux/commits/Alexey-Gladkov/ucounts-Fix-rlimit-max-values-check/20211126-224059
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6

in testcase: boot

on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):


+--------------------------------------------------------------------+------------+------------+
| | 98e4b47106 | dc7e5f9d41 |
+--------------------------------------------------------------------+------------+------------+
| boot_successes | 32 | 0 |
| boot_failures | 20 | 55 |
| UBSAN:array-index-out-of-bounds_in_kernel/ucount.c | 0 | 36 |
+--------------------------------------------------------------------+------------+------------+


If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>


[ 2.559359][ T1] UBSAN: array-index-out-of-bounds in kernel/ucount.c:109:33
[ 2.561796][ T1] index 13 is out of range for type 'long int [12]'
[ 2.563347][ T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.16.0-rc2-00002-gdc7e5f9d419c #1
[ 2.565651][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 2.567267][ T1] Call Trace:
[ 2.567267][ T1] dump_stack_lvl (lib/dump_stack.c:107)
[ 2.567267][ T1] dump_stack (lib/dump_stack.c:114)
[ 2.567267][ T1] ubsan_epilogue (lib/ubsan.c:152)
[ 2.567267][ T1] __ubsan_handle_out_of_bounds.cold (lib/ubsan.c:291 lib/ubsan.c:278)
[ 2.567267][ T1] ? kmemdup (mm/util.c:132)
[ 2.567267][ T1] setup_userns_sysctls (kernel/ucount.c:109)
[ 2.567267][ T1] ? idle_threads_init (kernel/ucount.c:359)
[ 2.567267][ T1] user_namespace_sysctl_init (kernel/ucount.c:371 (discriminator 2))
[ 2.567267][ T1] ? idle_threads_init (kernel/ucount.c:359)
[ 2.567267][ T1] do_one_initcall (init/main.c:1297)
[ 2.567267][ T1] ? __this_cpu_preempt_check (lib/smp_processor_id.c:67)
[ 2.567267][ T1] ? lock_is_held_type (kernel/locking/lockdep.c:438 kernel/locking/lockdep.c:5681)
[ 2.567267][ T1] ? rcu_read_lock_sched_held (include/linux/lockdep.h:283 kernel/rcu/update.c:125)
[ 2.567267][ T1] kernel_init_freeable (init/main.c:1369 init/main.c:1386 init/main.c:1405 init/main.c:1610)
[ 2.567267][ T1] ? rest_init (init/main.c:1491)
[ 2.567267][ T1] kernel_init (init/main.c:1501)
[ 2.567267][ T1] ret_from_fork (arch/x86/entry/entry_32.S:775)
[ 2.567347][ T1] ================================================================================
[ 2.570153][ T1] kprobes: kprobe jump-optimization is enabled. All kprobes are optimized if possible.
[ 2.571601][ T1] HugeTLB registered 2.00 MiB page size, pre-allocated 0 pages
[ 2.579937][ T1] ACPI: Added _OSI(Module Device)
[ 2.581413][ T1] ACPI: Added _OSI(Processor Device)
[ 2.582830][ T1] ACPI: Added _OSI(3.0 _SCP Extensions)
[ 2.583285][ T1] ACPI: Added _OSI(Processor Aggregator Device)
[ 2.584940][ T1] ACPI: Added _OSI(Linux-Dell-Video)
[ 2.586372][ T1] ACPI: Added _OSI(Linux-Lenovo-NV-HDMI-Audio)
[ 2.587290][ T1] ACPI: Added _OSI(Linux-HPI-Hybrid-Graphics)
[ 2.596108][ T1] ACPI: 1 ACPI AML tables successfully acquired and loaded
[ 2.599386][ T9] Callback from call_rcu_tasks() invoked.
[ 2.605503][ T1] ACPI: Interpreter enabled
[ 2.606945][ T1] ACPI: PM: (supports S0 S3 S4 S5)
[ 2.607284][ T1] ACPI: Using IOAPIC for interrupt routing
[ 2.609116][ T1] PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
[ 2.612350][ T1] ACPI: Enabled 2 GPEs in block 00 to 0F
[ 2.643022][ T1] ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
[ 2.643298][ T1] acpi PNP0A03:00: _OSC: OS supports [ASPM ClockPM Segments MSI HPX-Type3]
[ 2.645668][ T1] acpi PNP0A03:00: _OSC: not requesting OS control; OS requires [ExtendedConfig ASPM ClockPM MSI]
[ 2.647392][ T1] acpi PNP0A03:00: fail to add MMCONFIG information, can't access extended PCI configuration space under this bridge.
[ 2.654525][ T1] acpiphp: Slot [3] registered
[ 2.655430][ T1] acpiphp: Slot [4] registered
[ 2.656809][ T1] acpiphp: Slot [5] registered
[ 2.658207][ T1] acpiphp: Slot [6] registered
[ 2.659411][ T1] acpiphp: Slot [7] registered
[ 2.660914][ T1] acpiphp: Slot [8] registered
[ 2.662430][ T1] acpiphp: Slot [9] registered
[ 2.663430][ T1] acpiphp: Slot [10] registered
[ 2.664892][ T1] acpiphp: Slot [11] registered
[ 2.666269][ T1] acpiphp: Slot [12] registered
[ 2.667416][ T1] acpiphp: Slot [13] registered
[ 2.669191][ T1] acpiphp: Slot [14] registered
[ 2.670666][ T1] acpiphp: Slot [15] registered
[ 2.671408][ T1] acpiphp: Slot [16] registered
[ 2.672829][ T1] acpiphp: Slot [17] registered
[ 2.674311][ T1] acpiphp: Slot [18] registered
[ 2.675427][ T1] acpiphp: Slot [19] registered
[ 2.676842][ T1] acpiphp: Slot [20] registered
[ 2.678323][ T1] acpiphp: Slot [21] registered
[ 2.679419][ T1] acpiphp: Slot [22] registered
[ 2.680822][ T1] acpiphp: Slot [23] registered
[ 2.682305][ T1] acpiphp: Slot [24] registered
[ 2.683418][ T1] acpiphp: Slot [25] registered
[ 2.684928][ T1] acpiphp: Slot [26] registered
[ 2.686387][ T1] acpiphp: Slot [27] registered
[ 2.687409][ T1] acpiphp: Slot [28] registered
[ 2.688886][ T1] acpiphp: Slot [29] registered
[ 2.690319][ T1] acpiphp: Slot [30] registered
[ 2.691400][ T1] acpiphp: Slot [31] registered
[ 2.692745][ T1] PCI host bridge to bus 0000:00
[ 2.694068][ T1] pci_bus 0000:00: root bus resource [io 0x0000-0x0cf7 window]
[ 2.695285][ T1] pci_bus 0000:00: root bus resource [io 0x0d00-0xffff window]
[ 2.697287][ T1] pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000bffff window]
[ 2.699286][ T1] pci_bus 0000:00: root bus resource [mem 0xc0000000-0xfebfffff window]
[ 2.701613][ T1] pci_bus 0000:00: root bus resource [mem 0x140000000-0x1bfffffff window]
[ 2.703362][ T1] pci_bus 0000:00: root bus resource [bus 00-ff]
[ 2.705158][ T1] pci 0000:00:00.0: [8086:1237] type 00 class 0x060000
[ 2.708313][ T1] pci 0000:00:01.0: [8086:7000] type 00 class 0x060100
[ 2.711484][ T1] pci 0000:00:01.1: [8086:7010] type 00 class 0x010180
[ 2.718382][ T1] pci 0000:00:01.1: reg 0x20: [io 0xc040-0xc04f]
[ 2.721586][ T1] pci 0000:00:01.1: legacy IDE quirk: reg 0x10: [io 0x01f0-0x01f7]
[ 2.723286][ T1] pci 0000:00:01.1: legacy IDE quirk: reg 0x14: [io 0x03f6]
[ 2.725270][ T1] pci 0000:00:01.1: legacy IDE quirk: reg 0x18: [io 0x0170-0x0177]
[ 2.727282][ T1] pci 0000:00:01.1: legacy IDE quirk: reg 0x1c: [io 0x0376]
[ 2.729813][ T1] pci 0000:00:01.3: [8086:7113] type 00 class 0x068000
[ 2.731875][ T1] pci 0000:00:01.3: quirk: [io 0x0600-0x063f] claimed by PIIX4 ACPI
[ 2.735311][ T1] pci 0000:00:01.3: quirk: [io 0x0700-0x070f] claimed by PIIX4 SMB
[ 2.738460][ T1] pci 0000:00:02.0: [1234:1111] type 00 class 0x030000
[ 2.742150][ T1] pci 0000:00:02.0: reg 0x10: [mem 0xfd000000-0xfdffffff pref]
[ 2.747291][ T1] pci 0000:00:02.0: reg 0x18: [mem 0xfebf0000-0xfebf0fff]
[ 2.757450][ T1] pci 0000:00:02.0: reg 0x30: [mem 0xfebe0000-0xfebeffff pref]
[ 2.760228][ T1] pci 0000:00:03.0: [8086:100e] type 00 class 0x020000
[ 2.763282][ T1] pci 0000:00:03.0: reg 0x10: [mem 0xfebc0000-0xfebdffff]
[ 2.766577][ T1] pci 0000:00:03.0: reg 0x14: [io 0xc000-0xc03f]
[ 2.776442][ T1] pci 0000:00:03.0: reg 0x30: [mem 0xfeb80000-0xfebbffff pref]
[ 2.780153][ T1] pci 0000:00:04.0: [8086:25ab] type 00 class 0x088000
[ 2.782788][ T1] pci 0000:00:04.0: reg 0x10: [mem 0xfebf1000-0xfebf100f]
[ 2.789260][ T1] pci_bus 0000:00: on NUMA node 0


To reproduce:

# build kernel
cd linux
cp config-5.16.0-rc2-00002-gdc7e5f9d419c .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz


git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email

# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.



---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

Thanks,
Oliver Sang


Attachments:
(No filename) (9.05 kB)
config-5.16.0-rc2-00002-gdc7e5f9d419c (149.46 kB)
job-script (4.71 kB)
dmesg.xz (12.93 kB)
Download all attachments

2021-11-29 20:40:25

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v2 0/2] ucounts: Fix rlimit max values check

Checking the rlimit value specified in init_user_ns from the created userns does
not work properly. The issue is that the maximum value is taken by the same
rules as for ucounts. Because of this, we check the current rlimit counter value
with RLIM_INFINITY in init_user_ns.

v2:
- Fix a bug that was found by the lkp project.

--

Alexey Gladkov (2):
ucounts: Fix rlimit max values check
ucounts: Move rlimit max values from ucounts max

include/linux/user_namespace.h | 17 ++++++++++++-----
kernel/fork.c | 10 +++++-----
kernel/ucount.c | 19 +++++++++----------
kernel/user_namespace.c | 10 +++++-----
4 files changed, 31 insertions(+), 25 deletions(-)

--
2.33.0


2021-11-29 20:40:29

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v2 2/2] ucounts: Move rlimit max values from ucounts max

Since the semantics of maximum rlimit values are different, they need to
be separated from ucount_max. This will prevent the error of using
inc_count/dec_ucount for rlimit parameters.

This patch also renames the functions to emphasize the lack of
connection between rlimit_max and ucount_max.

v2:
- Fix the array-index-out-of-bounds that was found by the lkp project.

Reported-by: kernel test robot <[email protected]>
Signed-off-by: Alexey Gladkov <[email protected]>
---
include/linux/user_namespace.h | 17 ++++++++++++-----
kernel/fork.c | 10 +++++-----
kernel/ucount.c | 10 +++-------
kernel/user_namespace.c | 10 +++++-----
4 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 33a4240e6a6f..812b6935f4a3 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -58,10 +58,11 @@ enum ucount_type {
UCOUNT_RLIMIT_MSGQUEUE,
UCOUNT_RLIMIT_SIGPENDING,
UCOUNT_RLIMIT_MEMLOCK,
- UCOUNT_COUNTS,
+ UCOUNT_ALL_COUNTS,
};

-#define MAX_PER_NAMESPACE_UCOUNTS UCOUNT_RLIMIT_NPROC
+#define UCOUNT_COUNTS UCOUNT_RLIMIT_NPROC
+#define RLIMIT_COUNTS UCOUNT_ALL_COUNTS - UCOUNT_COUNTS + 1

struct user_namespace {
struct uid_gid_map uid_map;
@@ -99,6 +100,7 @@ struct user_namespace {
#endif
struct ucounts *ucounts;
long ucount_max[UCOUNT_COUNTS];
+ long rlimit_max[RLIMIT_COUNTS];
} __randomize_layout;

struct ucounts {
@@ -106,7 +108,7 @@ struct ucounts {
struct user_namespace *ns;
kuid_t uid;
atomic_t count;
- atomic_long_t ucount[UCOUNT_COUNTS];
+ atomic_long_t ucount[UCOUNT_ALL_COUNTS];
};

extern struct user_namespace init_user_ns;
@@ -131,10 +133,15 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type);
void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type);
bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max);

-static inline void set_rlimit_ucount_max(struct user_namespace *ns,
+static inline long get_userns_rlimit_max(struct user_namespace *ns, enum ucount_type type)
+{
+ return READ_ONCE(ns->rlimit_max[type - UCOUNT_COUNTS]);
+}
+
+static inline void set_userns_rlimit_max(struct user_namespace *ns,
enum ucount_type type, unsigned long max)
{
- ns->ucount_max[type] = max <= LONG_MAX ? max : LONG_MAX;
+ ns->rlimit_max[type - UCOUNT_COUNTS] = max <= LONG_MAX ? max : LONG_MAX;
}

#ifdef CONFIG_USER_NS
diff --git a/kernel/fork.c b/kernel/fork.c
index 3244cc56b697..365819458030 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -836,13 +836,13 @@ void __init fork_init(void)
init_task.signal->rlim[RLIMIT_SIGPENDING] =
init_task.signal->rlim[RLIMIT_NPROC];

- for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++)
+ for (i = 0; i < UCOUNT_COUNTS; i++)
init_user_ns.ucount_max[i] = max_threads/2;

- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);

#ifdef CONFIG_VMAP_STACK
cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "fork:vm_stack_cache",
diff --git a/kernel/ucount.c b/kernel/ucount.c
index 7b32c356ebc5..ffffcfae8474 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -87,10 +87,6 @@ static struct ctl_table user_table[] = {
UCOUNT_ENTRY("max_fanotify_groups"),
UCOUNT_ENTRY("max_fanotify_marks"),
#endif
- { },
- { },
- { },
- { },
{ }
};
#endif /* CONFIG_SYSCTL */
@@ -273,7 +269,7 @@ long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
ret = LONG_MAX;
else if (iter == ucounts)
ret = new;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
}
return ret;
}
@@ -322,7 +318,7 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
goto unwind;
if (iter == ucounts)
ret = new;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
/*
* Grab an extra ucount reference for the caller when
* the rlimit count was previously 0.
@@ -350,7 +346,7 @@ bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsign
for (iter = ucounts; iter; iter = iter->ns->ucounts) {
if (get_ucounts_value(iter, type) > max)
return true;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
}
return false;
}
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 6b2e3ca7ee99..b9f6729b4e5f 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -119,13 +119,13 @@ int create_user_ns(struct cred *new)
ns->owner = owner;
ns->group = group;
INIT_WORK(&ns->work, free_user_ns);
- for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++) {
+ for (i = 0; i < UCOUNT_COUNTS; i++) {
ns->ucount_max[i] = INT_MAX;
}
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
ns->ucounts = ucounts;

/* Inherit USERNS_SETGROUPS_ALLOWED from our parent */
--
2.33.0


2021-11-29 20:40:32

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v2 1/2] ucounts: Fix rlimit max values check

The semantics of the rlimit max values differs from ucounts itself. When
creating a new userns, we store the current rlimit of the process in
ucount_max. Thus, the value of the limit in the parent userns is saved
in the created one.

The problem is that now we are taking the maximum value for counter from
the same userns. So for init_user_ns it will always be RLIM_INFINITY.

To fix the problem we need to check the counter value with the max value
stored in userns.

Reproducer:

su - test -c "ulimit -u 3; sleep 5 & sleep 6 & unshare -U --map-root-user sh -c 'sleep 7 & sleep 8 & date; wait'"

Before:

[1] 175
[2] 176
Fri Nov 26 13:48:20 UTC 2021
[1]- Done sleep 5
[2]+ Done sleep 6

After:

[1] 167
[2] 168
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: retry: Resource temporarily unavailable
sh: fork: Interrupted system call
[1]- Done sleep 5
[2]+ Done sleep 6

Fixes: c54b245d0118 ("Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace")
Reported-by: Gleb Fotengauer-Malinovskiy <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
Signed-off-by: Alexey Gladkov <[email protected]>
---
kernel/ucount.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/ucount.c b/kernel/ucount.c
index 4f5613dac227..7b32c356ebc5 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -264,15 +264,16 @@ void dec_ucount(struct ucounts *ucounts, enum ucount_type type)
long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
{
struct ucounts *iter;
+ long max = LONG_MAX;
long ret = 0;

for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- long max = READ_ONCE(iter->ns->ucount_max[type]);
long new = atomic_long_add_return(v, &iter->ucount[type]);
if (new < 0 || new > max)
ret = LONG_MAX;
else if (iter == ucounts)
ret = new;
+ max = READ_ONCE(iter->ns->ucount_max[type]);
}
return ret;
}
@@ -312,15 +313,16 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
{
/* Caller must hold a reference to ucounts */
struct ucounts *iter;
+ long max = LONG_MAX;
long dec, ret = 0;

for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- long max = READ_ONCE(iter->ns->ucount_max[type]);
long new = atomic_long_add_return(1, &iter->ucount[type]);
if (new < 0 || new > max)
goto unwind;
if (iter == ucounts)
ret = new;
+ max = READ_ONCE(iter->ns->ucount_max[type]);
/*
* Grab an extra ucount reference for the caller when
* the rlimit count was previously 0.
@@ -339,15 +341,16 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
return 0;
}

-bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max)
+bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long rlimit)
{
struct ucounts *iter;
- if (get_ucounts_value(ucounts, type) > max)
- return true;
+ long max = rlimit;
+ if (rlimit > LONG_MAX)
+ max = LONG_MAX;
for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- max = READ_ONCE(iter->ns->ucount_max[type]);
if (get_ucounts_value(iter, type) > max)
return true;
+ max = READ_ONCE(iter->ns->ucount_max[type]);
}
return false;
}
--
2.33.0


2021-12-03 13:33:58

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] ucounts: Move rlimit max values from ucounts max

Hi Alexey,

url: https://github.com/0day-ci/linux/commits/Alexey-Gladkov/ucounts-Fix-rlimit-max-values-check/20211126-224059
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: i386-randconfig-m021-20211126 (https://download.01.org/0day-ci/archive/20211128/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>

smatch warnings:
kernel/ucount.c:109 setup_userns_sysctls() error: buffer overflow 'ns->ucount_max' 10 <= 13

vim +109 kernel/ucount.c

dbec28460a89aa Eric W. Biederman 2016-07-30 98 bool setup_userns_sysctls(struct user_namespace *ns)
dbec28460a89aa Eric W. Biederman 2016-07-30 99 {
dbec28460a89aa Eric W. Biederman 2016-07-30 100 #ifdef CONFIG_SYSCTL
dbec28460a89aa Eric W. Biederman 2016-07-30 101 struct ctl_table *tbl;
0f538e3e712a51 Jan Kara 2020-04-07 102
0f538e3e712a51 Jan Kara 2020-04-07 103 BUILD_BUG_ON(ARRAY_SIZE(user_table) != UCOUNT_COUNTS + 1);
dbec28460a89aa Eric W. Biederman 2016-07-30 104 setup_sysctl_set(&ns->set, &set_root, set_is_seen);
f6b2db1a3e8d14 Eric W. Biederman 2016-08-08 105 tbl = kmemdup(user_table, sizeof(user_table), GFP_KERNEL);
dbec28460a89aa Eric W. Biederman 2016-07-30 106 if (tbl) {
25f9c0817c535a Eric W. Biederman 2016-08-08 107 int i;
25f9c0817c535a Eric W. Biederman 2016-08-08 108 for (i = 0; i < UCOUNT_COUNTS; i++) {
25f9c0817c535a Eric W. Biederman 2016-08-08 @109 tbl[i].data = &ns->ucount_max[i];

The patch changes the size of ->ucount_max[] to MAX_PER_NAMESPACE_UCOUNTS
but this loop still goes up to UCOUNT_COUNTS.

25f9c0817c535a Eric W. Biederman 2016-08-08 110 }
f6b2db1a3e8d14 Eric W. Biederman 2016-08-08 111 ns->sysctls = __register_sysctl_table(&ns->set, "user", tbl);
dbec28460a89aa Eric W. Biederman 2016-07-30 112 }
dbec28460a89aa Eric W. Biederman 2016-07-30 113 if (!ns->sysctls) {
dbec28460a89aa Eric W. Biederman 2016-07-30 114 kfree(tbl);
dbec28460a89aa Eric W. Biederman 2016-07-30 115 retire_sysctl_set(&ns->set);
dbec28460a89aa Eric W. Biederman 2016-07-30 116 return false;
dbec28460a89aa Eric W. Biederman 2016-07-30 117 }
dbec28460a89aa Eric W. Biederman 2016-07-30 118 #endif
dbec28460a89aa Eric W. Biederman 2016-07-30 119 return true;
dbec28460a89aa Eric W. Biederman 2016-07-30 120 }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


2021-12-03 13:54:58

by Alexey Gladkov

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] ucounts: Move rlimit max values from ucounts max

On Fri, Dec 03, 2021 at 04:33:25PM +0300, Dan Carpenter wrote:
> Hi Alexey,
>
> url: https://github.com/0day-ci/linux/commits/Alexey-Gladkov/ucounts-Fix-rlimit-max-values-check/20211126-224059
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
> config: i386-randconfig-m021-20211126 (https://download.01.org/0day-ci/archive/20211128/[email protected]/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> smatch warnings:
> kernel/ucount.c:109 setup_userns_sysctls() error: buffer overflow 'ns->ucount_max' 10 <= 13
>
> vim +109 kernel/ucount.c
>
> dbec28460a89aa Eric W. Biederman 2016-07-30 98 bool setup_userns_sysctls(struct user_namespace *ns)
> dbec28460a89aa Eric W. Biederman 2016-07-30 99 {
> dbec28460a89aa Eric W. Biederman 2016-07-30 100 #ifdef CONFIG_SYSCTL
> dbec28460a89aa Eric W. Biederman 2016-07-30 101 struct ctl_table *tbl;
> 0f538e3e712a51 Jan Kara 2020-04-07 102
> 0f538e3e712a51 Jan Kara 2020-04-07 103 BUILD_BUG_ON(ARRAY_SIZE(user_table) != UCOUNT_COUNTS + 1);
> dbec28460a89aa Eric W. Biederman 2016-07-30 104 setup_sysctl_set(&ns->set, &set_root, set_is_seen);
> f6b2db1a3e8d14 Eric W. Biederman 2016-08-08 105 tbl = kmemdup(user_table, sizeof(user_table), GFP_KERNEL);
> dbec28460a89aa Eric W. Biederman 2016-07-30 106 if (tbl) {
> 25f9c0817c535a Eric W. Biederman 2016-08-08 107 int i;
> 25f9c0817c535a Eric W. Biederman 2016-08-08 108 for (i = 0; i < UCOUNT_COUNTS; i++) {
> 25f9c0817c535a Eric W. Biederman 2016-08-08 @109 tbl[i].data = &ns->ucount_max[i];
>
> The patch changes the size of ->ucount_max[] to MAX_PER_NAMESPACE_UCOUNTS
> but this loop still goes up to UCOUNT_COUNTS.
>
> 25f9c0817c535a Eric W. Biederman 2016-08-08 110 }
> f6b2db1a3e8d14 Eric W. Biederman 2016-08-08 111 ns->sysctls = __register_sysctl_table(&ns->set, "user", tbl);
> dbec28460a89aa Eric W. Biederman 2016-07-30 112 }
> dbec28460a89aa Eric W. Biederman 2016-07-30 113 if (!ns->sysctls) {
> dbec28460a89aa Eric W. Biederman 2016-07-30 114 kfree(tbl);
> dbec28460a89aa Eric W. Biederman 2016-07-30 115 retire_sysctl_set(&ns->set);
> dbec28460a89aa Eric W. Biederman 2016-07-30 116 return false;
> dbec28460a89aa Eric W. Biederman 2016-07-30 117 }
> dbec28460a89aa Eric W. Biederman 2016-07-30 118 #endif
> dbec28460a89aa Eric W. Biederman 2016-07-30 119 return true;
> dbec28460a89aa Eric W. Biederman 2016-07-30 120 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
>

Thanks! But a few days ago I already post a new version of this changeset
with fix:

https://lore.kernel.org/containers/24c87e225c7950bf2ea1ff4b4a8f237348808241.1638218242.git.legion@kernel.org/

--
Rgrds, legion


2021-12-03 13:57:46

by Alexey Gladkov

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] ucounts: Move rlimit max values from ucounts max

On Fri, Dec 03, 2021 at 04:33:25PM +0300, Dan Carpenter wrote:
> Hi Alexey,
>
> url: https://github.com/0day-ci/linux/commits/Alexey-Gladkov/ucounts-Fix-rlimit-max-values-check/20211126-224059
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
> config: i386-randconfig-m021-20211126 (https://download.01.org/0day-ci/archive/20211128/[email protected]/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> smatch warnings:
> kernel/ucount.c:109 setup_userns_sysctls() error: buffer overflow 'ns->ucount_max' 10 <= 13
>
> vim +109 kernel/ucount.c
>
> dbec28460a89aa Eric W. Biederman 2016-07-30 98 bool setup_userns_sysctls(struct user_namespace *ns)
> dbec28460a89aa Eric W. Biederman 2016-07-30 99 {
> dbec28460a89aa Eric W. Biederman 2016-07-30 100 #ifdef CONFIG_SYSCTL
> dbec28460a89aa Eric W. Biederman 2016-07-30 101 struct ctl_table *tbl;
> 0f538e3e712a51 Jan Kara 2020-04-07 102
> 0f538e3e712a51 Jan Kara 2020-04-07 103 BUILD_BUG_ON(ARRAY_SIZE(user_table) != UCOUNT_COUNTS + 1);
> dbec28460a89aa Eric W. Biederman 2016-07-30 104 setup_sysctl_set(&ns->set, &set_root, set_is_seen);
> f6b2db1a3e8d14 Eric W. Biederman 2016-08-08 105 tbl = kmemdup(user_table, sizeof(user_table), GFP_KERNEL);
> dbec28460a89aa Eric W. Biederman 2016-07-30 106 if (tbl) {
> 25f9c0817c535a Eric W. Biederman 2016-08-08 107 int i;
> 25f9c0817c535a Eric W. Biederman 2016-08-08 108 for (i = 0; i < UCOUNT_COUNTS; i++) {
> 25f9c0817c535a Eric W. Biederman 2016-08-08 @109 tbl[i].data = &ns->ucount_max[i];
>
> The patch changes the size of ->ucount_max[] to MAX_PER_NAMESPACE_UCOUNTS
> but this loop still goes up to UCOUNT_COUNTS.
>
> 25f9c0817c535a Eric W. Biederman 2016-08-08 110 }
> f6b2db1a3e8d14 Eric W. Biederman 2016-08-08 111 ns->sysctls = __register_sysctl_table(&ns->set, "user", tbl);
> dbec28460a89aa Eric W. Biederman 2016-07-30 112 }
> dbec28460a89aa Eric W. Biederman 2016-07-30 113 if (!ns->sysctls) {
> dbec28460a89aa Eric W. Biederman 2016-07-30 114 kfree(tbl);
> dbec28460a89aa Eric W. Biederman 2016-07-30 115 retire_sysctl_set(&ns->set);
> dbec28460a89aa Eric W. Biederman 2016-07-30 116 return false;
> dbec28460a89aa Eric W. Biederman 2016-07-30 117 }
> dbec28460a89aa Eric W. Biederman 2016-07-30 118 #endif
> dbec28460a89aa Eric W. Biederman 2016-07-30 119 return true;
> dbec28460a89aa Eric W. Biederman 2016-07-30 120 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
>

Thanks! But a few days ago I already post a new version of this changeset
with fix:

https://lore.kernel.org/containers/24c87e225c7950bf2ea1ff4b4a8f237348808241.1638218242.git.legion@kernel.org/

--
Rgrds, legion


2021-12-03 14:20:02

by David Laight

[permalink] [raw]
Subject: RE: [PATCH v1 2/2] ucounts: Move rlimit max values from ucounts max

From: Alexey Gladkov
> Sent: 03 December 2021 13:55
...
> > 25f9c0817c535a Eric W. Biederman 2016-08-08 108 for (i = 0; i < UCOUNT_COUNTS; i++) {
> > 25f9c0817c535a Eric W. Biederman 2016-08-08 @109 tbl[i].data = &ns->ucount_max[i];
> >
> > The patch changes the size of ->ucount_max[] to MAX_PER_NAMESPACE_UCOUNTS
> > but this loop still goes up to UCOUNT_COUNTS.
> >
> >
>
> Thanks! But a few days ago I already post a new version of this changeset
> with fix:
>
> https://lore.kernel.org/containers/24c87e225c7950bf2ea1ff4b4a8f237348808241.1638218242.git.legion@kernel.org/

Use ARRAY_SIZE() so that it 'just doesn't go wrong'.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


2021-12-13 15:50:57

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ucounts: Move rlimit max values from ucounts max

Alexey Gladkov <[email protected]> writes:

> Since the semantics of maximum rlimit values are different, they need to
> be separated from ucount_max. This will prevent the error of using
> inc_count/dec_ucount for rlimit parameters.
>
> This patch also renames the functions to emphasize the lack of
> connection between rlimit_max and ucount_max.
>
> v2:
> - Fix the array-index-out-of-bounds that was found by the lkp project.

Just looking at this I wonder if it things would come out cleaner if we
had a separate "enum ucount_rlimit_type" and a separate array in "struct
ucount" for the rlimits.

Just so that people don't accidentally mix the two.

>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Alexey Gladkov <[email protected]>
> ---
> include/linux/user_namespace.h | 17 ++++++++++++-----
> kernel/fork.c | 10 +++++-----
> kernel/ucount.c | 10 +++-------
> kernel/user_namespace.c | 10 +++++-----
> 4 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 33a4240e6a6f..812b6935f4a3 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -58,10 +58,11 @@ enum ucount_type {
> UCOUNT_RLIMIT_MSGQUEUE,
> UCOUNT_RLIMIT_SIGPENDING,
> UCOUNT_RLIMIT_MEMLOCK,
> - UCOUNT_COUNTS,
> + UCOUNT_ALL_COUNTS,
> };
>
> -#define MAX_PER_NAMESPACE_UCOUNTS UCOUNT_RLIMIT_NPROC
> +#define UCOUNT_COUNTS UCOUNT_RLIMIT_NPROC
> +#define RLIMIT_COUNTS UCOUNT_ALL_COUNTS - UCOUNT_COUNTS + 1
>
> struct user_namespace {
> struct uid_gid_map uid_map;
> @@ -99,6 +100,7 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> long ucount_max[UCOUNT_COUNTS];
> + long rlimit_max[RLIMIT_COUNTS];
> } __randomize_layout;
>
> struct ucounts {
> @@ -106,7 +108,7 @@ struct ucounts {
> struct user_namespace *ns;
> kuid_t uid;
> atomic_t count;
> - atomic_long_t ucount[UCOUNT_COUNTS];
> + atomic_long_t ucount[UCOUNT_ALL_COUNTS];
> };
>
> extern struct user_namespace init_user_ns;
> @@ -131,10 +133,15 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type);
> void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type);
> bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max);
>
> -static inline void set_rlimit_ucount_max(struct user_namespace *ns,
> +static inline long get_userns_rlimit_max(struct user_namespace *ns, enum ucount_type type)
> +{
> + return READ_ONCE(ns->rlimit_max[type - UCOUNT_COUNTS]);
> +}
> +
> +static inline void set_userns_rlimit_max(struct user_namespace *ns,
> enum ucount_type type, unsigned long max)
> {
> - ns->ucount_max[type] = max <= LONG_MAX ? max : LONG_MAX;
> + ns->rlimit_max[type - UCOUNT_COUNTS] = max <= LONG_MAX ? max : LONG_MAX;
> }
>
> #ifdef CONFIG_USER_NS
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 3244cc56b697..365819458030 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -836,13 +836,13 @@ void __init fork_init(void)
> init_task.signal->rlim[RLIMIT_SIGPENDING] =
> init_task.signal->rlim[RLIMIT_NPROC];
>
> - for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++)
> + for (i = 0; i < UCOUNT_COUNTS; i++)
> init_user_ns.ucount_max[i] = max_threads/2;
>
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
>
> #ifdef CONFIG_VMAP_STACK
> cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "fork:vm_stack_cache",
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index 7b32c356ebc5..ffffcfae8474 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -87,10 +87,6 @@ static struct ctl_table user_table[] = {
> UCOUNT_ENTRY("max_fanotify_groups"),
> UCOUNT_ENTRY("max_fanotify_marks"),
> #endif
> - { },
> - { },
> - { },
> - { },
> { }
> };
> #endif /* CONFIG_SYSCTL */
> @@ -273,7 +269,7 @@ long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
> ret = LONG_MAX;
> else if (iter == ucounts)
> ret = new;
> - max = READ_ONCE(iter->ns->ucount_max[type]);
> + max = get_userns_rlimit_max(iter->ns, type);
> }
> return ret;
> }
> @@ -322,7 +318,7 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
> goto unwind;
> if (iter == ucounts)
> ret = new;
> - max = READ_ONCE(iter->ns->ucount_max[type]);
> + max = get_userns_rlimit_max(iter->ns, type);
> /*
> * Grab an extra ucount reference for the caller when
> * the rlimit count was previously 0.
> @@ -350,7 +346,7 @@ bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsign
> for (iter = ucounts; iter; iter = iter->ns->ucounts) {
> if (get_ucounts_value(iter, type) > max)
> return true;
> - max = READ_ONCE(iter->ns->ucount_max[type]);
> + max = get_userns_rlimit_max(iter->ns, type);
> }
> return false;
> }
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 6b2e3ca7ee99..b9f6729b4e5f 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -119,13 +119,13 @@ int create_user_ns(struct cred *new)
> ns->owner = owner;
> ns->group = group;
> INIT_WORK(&ns->work, free_user_ns);
> - for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++) {
> + for (i = 0; i < UCOUNT_COUNTS; i++) {
> ns->ucount_max[i] = INT_MAX;
> }
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
> ns->ucounts = ucounts;
>
> /* Inherit USERNS_SETGROUPS_ALLOWED from our parent */

2021-12-17 14:48:40

by Alexey Gladkov

[permalink] [raw]
Subject: [PATCH v3] ucounts: Split rlimit and ucount values and max values

Since the semantics of maximum rlimit values are different, it would be
better not to mix ucount and rlimit values. This will prevent the error
of using inc_count/dec_ucount for rlimit parameters.

This patch also renames the functions to emphasize the lack of
connection between rlimit and ucount.

v2:
- Fix the array-index-out-of-bounds that was found by the lkp project.

Reported-by: kernel test robot <[email protected]>
Signed-off-by: Alexey Gladkov <[email protected]>
---
fs/exec.c | 2 +-
fs/proc/array.c | 2 +-
include/linux/user_namespace.h | 35 +++++++++++++++++++++-------------
kernel/fork.c | 12 ++++++------
kernel/sys.c | 2 +-
kernel/ucount.c | 24 ++++++++++-------------
kernel/user_namespace.c | 10 +++++-----
7 files changed, 46 insertions(+), 41 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 537d92c41105..d3f769837058 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1877,7 +1877,7 @@ static int do_execveat_common(int fd, struct filename *filename,
* whether NPROC limit is still exceeded.
*/
if ((current->flags & PF_NPROC_EXCEEDED) &&
- is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
+ is_rlimit_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
retval = -EAGAIN;
goto out_ret;
}
diff --git a/fs/proc/array.c b/fs/proc/array.c
index ff869a66b34e..d3fa539c755e 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -274,7 +274,7 @@ static inline void task_sig(struct seq_file *m, struct task_struct *p)
collect_sigign_sigcatch(p, &ignored, &caught);
num_threads = get_nr_threads(p);
rcu_read_lock(); /* FIXME: is this correct? */
- qsize = get_ucounts_value(task_ucounts(p), UCOUNT_RLIMIT_SIGPENDING);
+ qsize = get_rlimit_value(task_ucounts(p), UCOUNT_RLIMIT_SIGPENDING);
rcu_read_unlock();
qlim = task_rlimit(p, RLIMIT_SIGPENDING);
unlock_task_sighand(p, &flags);
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 33a4240e6a6f..45f09bec02c4 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -54,15 +54,17 @@ enum ucount_type {
UCOUNT_FANOTIFY_GROUPS,
UCOUNT_FANOTIFY_MARKS,
#endif
+ UCOUNT_COUNTS,
+};
+
+enum rlimit_type {
UCOUNT_RLIMIT_NPROC,
UCOUNT_RLIMIT_MSGQUEUE,
UCOUNT_RLIMIT_SIGPENDING,
UCOUNT_RLIMIT_MEMLOCK,
- UCOUNT_COUNTS,
+ UCOUNT_RLIMIT_COUNTS,
};

-#define MAX_PER_NAMESPACE_UCOUNTS UCOUNT_RLIMIT_NPROC
-
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -99,6 +101,7 @@ struct user_namespace {
#endif
struct ucounts *ucounts;
long ucount_max[UCOUNT_COUNTS];
+ long rlimit_max[UCOUNT_RLIMIT_COUNTS];
} __randomize_layout;

struct ucounts {
@@ -107,6 +110,7 @@ struct ucounts {
kuid_t uid;
atomic_t count;
atomic_long_t ucount[UCOUNT_COUNTS];
+ atomic_long_t rlimit[UCOUNT_RLIMIT_COUNTS];
};

extern struct user_namespace init_user_ns;
@@ -120,21 +124,26 @@ struct ucounts *alloc_ucounts(struct user_namespace *ns, kuid_t uid);
struct ucounts * __must_check get_ucounts(struct ucounts *ucounts);
void put_ucounts(struct ucounts *ucounts);

-static inline long get_ucounts_value(struct ucounts *ucounts, enum ucount_type type)
+static inline long get_rlimit_value(struct ucounts *ucounts, enum rlimit_type type)
{
- return atomic_long_read(&ucounts->ucount[type]);
+ return atomic_long_read(&ucounts->rlimit[type]);
}

-long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v);
-bool dec_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v);
-long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type);
-void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type);
-bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max);
+long inc_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v);
+bool dec_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v);
+long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type);
+void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum rlimit_type type);
+bool is_rlimit_overlimit(struct ucounts *ucounts, enum rlimit_type type, unsigned long max);
+
+static inline long get_userns_rlimit_max(struct user_namespace *ns, enum rlimit_type type)
+{
+ return READ_ONCE(ns->rlimit_max[type]);
+}

-static inline void set_rlimit_ucount_max(struct user_namespace *ns,
- enum ucount_type type, unsigned long max)
+static inline void set_userns_rlimit_max(struct user_namespace *ns,
+ enum rlimit_type type, unsigned long max)
{
- ns->ucount_max[type] = max <= LONG_MAX ? max : LONG_MAX;
+ ns->rlimit_max[type] = max <= LONG_MAX ? max : LONG_MAX;
}

#ifdef CONFIG_USER_NS
diff --git a/kernel/fork.c b/kernel/fork.c
index 3244cc56b697..fb68d56bcd7e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -836,13 +836,13 @@ void __init fork_init(void)
init_task.signal->rlim[RLIMIT_SIGPENDING] =
init_task.signal->rlim[RLIMIT_NPROC];

- for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++)
+ for (i = 0; i < UCOUNT_COUNTS; i++)
init_user_ns.ucount_max[i] = max_threads/2;

- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
- set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
+ set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);

#ifdef CONFIG_VMAP_STACK
cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "fork:vm_stack_cache",
@@ -2053,7 +2053,7 @@ static __latent_entropy struct task_struct *copy_process(
DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
#endif
retval = -EAGAIN;
- if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
+ if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
if (p->real_cred->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_free;
diff --git a/kernel/sys.c b/kernel/sys.c
index 8fdac0d90504..2c86d245acd6 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -479,7 +479,7 @@ static int set_user(struct cred *new)
* for programs doing set*uid()+execve() by harmlessly deferring the
* failure to the execve() stage.
*/
- if (is_ucounts_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
+ if (is_rlimit_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
new_user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
current->flags |= PF_NPROC_EXCEEDED;
diff --git a/kernel/ucount.c b/kernel/ucount.c
index 7b32c356ebc5..22070f004e97 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -87,10 +87,6 @@ static struct ctl_table user_table[] = {
UCOUNT_ENTRY("max_fanotify_groups"),
UCOUNT_ENTRY("max_fanotify_marks"),
#endif
- { },
- { },
- { },
- { },
{ }
};
#endif /* CONFIG_SYSCTL */
@@ -261,7 +257,7 @@ void dec_ucount(struct ucounts *ucounts, enum ucount_type type)
put_ucounts(ucounts);
}

-long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
+long inc_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v)
{
struct ucounts *iter;
long max = LONG_MAX;
@@ -273,12 +269,12 @@ long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
ret = LONG_MAX;
else if (iter == ucounts)
ret = new;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
}
return ret;
}

-bool dec_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
+bool dec_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v)
{
struct ucounts *iter;
long new = -1; /* Silence compiler warning */
@@ -292,7 +288,7 @@ bool dec_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
}

static void do_dec_rlimit_put_ucounts(struct ucounts *ucounts,
- struct ucounts *last, enum ucount_type type)
+ struct ucounts *last, enum rlimit_type type)
{
struct ucounts *iter, *next;
for (iter = ucounts; iter != last; iter = next) {
@@ -304,12 +300,12 @@ static void do_dec_rlimit_put_ucounts(struct ucounts *ucounts,
}
}

-void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type)
+void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum rlimit_type type)
{
do_dec_rlimit_put_ucounts(ucounts, NULL, type);
}

-long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
+long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type)
{
/* Caller must hold a reference to ucounts */
struct ucounts *iter;
@@ -322,7 +318,7 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
goto unwind;
if (iter == ucounts)
ret = new;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
/*
* Grab an extra ucount reference for the caller when
* the rlimit count was previously 0.
@@ -341,16 +337,16 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
return 0;
}

-bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long rlimit)
+bool is_rlimit_overlimit(struct ucounts *ucounts, enum rlimit_type type, unsigned long rlimit)
{
struct ucounts *iter;
long max = rlimit;
if (rlimit > LONG_MAX)
max = LONG_MAX;
for (iter = ucounts; iter; iter = iter->ns->ucounts) {
- if (get_ucounts_value(iter, type) > max)
+ if (get_rlimit_value(iter, type) > max)
return true;
- max = READ_ONCE(iter->ns->ucount_max[type]);
+ max = get_userns_rlimit_max(iter->ns, type);
}
return false;
}
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 6b2e3ca7ee99..b9f6729b4e5f 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -119,13 +119,13 @@ int create_user_ns(struct cred *new)
ns->owner = owner;
ns->group = group;
INIT_WORK(&ns->work, free_user_ns);
- for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++) {
+ for (i = 0; i < UCOUNT_COUNTS; i++) {
ns->ucount_max[i] = INT_MAX;
}
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
- set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
+ set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
ns->ucounts = ucounts;

/* Inherit USERNS_SETGROUPS_ALLOWED from our parent */
--
2.33.0


2021-12-19 19:55:22

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH v3] ucounts: Split rlimit and ucount values and max values

Alexey Gladkov <[email protected]> writes:

> Since the semantics of maximum rlimit values are different, it would be
> better not to mix ucount and rlimit values. This will prevent the error
> of using inc_count/dec_ucount for rlimit parameters.
>
> This patch also renames the functions to emphasize the lack of
> connection between rlimit and ucount.
>
> v2:
> - Fix the array-index-out-of-bounds that was found by the lkp project.

At a quick read through this looks good.

I will see about getting this into for-next so we can merge this for v5.17

Eric

>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Alexey Gladkov <[email protected]>
> ---
> fs/exec.c | 2 +-
> fs/proc/array.c | 2 +-
> include/linux/user_namespace.h | 35 +++++++++++++++++++++-------------
> kernel/fork.c | 12 ++++++------
> kernel/sys.c | 2 +-
> kernel/ucount.c | 24 ++++++++++-------------
> kernel/user_namespace.c | 10 +++++-----
> 7 files changed, 46 insertions(+), 41 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 537d92c41105..d3f769837058 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1877,7 +1877,7 @@ static int do_execveat_common(int fd, struct filename *filename,
> * whether NPROC limit is still exceeded.
> */
> if ((current->flags & PF_NPROC_EXCEEDED) &&
> - is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> + is_rlimit_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> retval = -EAGAIN;
> goto out_ret;
> }
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index ff869a66b34e..d3fa539c755e 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -274,7 +274,7 @@ static inline void task_sig(struct seq_file *m, struct task_struct *p)
> collect_sigign_sigcatch(p, &ignored, &caught);
> num_threads = get_nr_threads(p);
> rcu_read_lock(); /* FIXME: is this correct? */
> - qsize = get_ucounts_value(task_ucounts(p), UCOUNT_RLIMIT_SIGPENDING);
> + qsize = get_rlimit_value(task_ucounts(p), UCOUNT_RLIMIT_SIGPENDING);
> rcu_read_unlock();
> qlim = task_rlimit(p, RLIMIT_SIGPENDING);
> unlock_task_sighand(p, &flags);
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 33a4240e6a6f..45f09bec02c4 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -54,15 +54,17 @@ enum ucount_type {
> UCOUNT_FANOTIFY_GROUPS,
> UCOUNT_FANOTIFY_MARKS,
> #endif
> + UCOUNT_COUNTS,
> +};
> +
> +enum rlimit_type {
> UCOUNT_RLIMIT_NPROC,
> UCOUNT_RLIMIT_MSGQUEUE,
> UCOUNT_RLIMIT_SIGPENDING,
> UCOUNT_RLIMIT_MEMLOCK,
> - UCOUNT_COUNTS,
> + UCOUNT_RLIMIT_COUNTS,
> };
>
> -#define MAX_PER_NAMESPACE_UCOUNTS UCOUNT_RLIMIT_NPROC
> -
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -99,6 +101,7 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> long ucount_max[UCOUNT_COUNTS];
> + long rlimit_max[UCOUNT_RLIMIT_COUNTS];
> } __randomize_layout;
>
> struct ucounts {
> @@ -107,6 +110,7 @@ struct ucounts {
> kuid_t uid;
> atomic_t count;
> atomic_long_t ucount[UCOUNT_COUNTS];
> + atomic_long_t rlimit[UCOUNT_RLIMIT_COUNTS];
> };
>
> extern struct user_namespace init_user_ns;
> @@ -120,21 +124,26 @@ struct ucounts *alloc_ucounts(struct user_namespace *ns, kuid_t uid);
> struct ucounts * __must_check get_ucounts(struct ucounts *ucounts);
> void put_ucounts(struct ucounts *ucounts);
>
> -static inline long get_ucounts_value(struct ucounts *ucounts, enum ucount_type type)
> +static inline long get_rlimit_value(struct ucounts *ucounts, enum rlimit_type type)
> {
> - return atomic_long_read(&ucounts->ucount[type]);
> + return atomic_long_read(&ucounts->rlimit[type]);
> }
>
> -long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v);
> -bool dec_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v);
> -long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type);
> -void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type);
> -bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long max);
> +long inc_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v);
> +bool dec_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v);
> +long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type);
> +void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum rlimit_type type);
> +bool is_rlimit_overlimit(struct ucounts *ucounts, enum rlimit_type type, unsigned long max);
> +
> +static inline long get_userns_rlimit_max(struct user_namespace *ns, enum rlimit_type type)
> +{
> + return READ_ONCE(ns->rlimit_max[type]);
> +}
>
> -static inline void set_rlimit_ucount_max(struct user_namespace *ns,
> - enum ucount_type type, unsigned long max)
> +static inline void set_userns_rlimit_max(struct user_namespace *ns,
> + enum rlimit_type type, unsigned long max)
> {
> - ns->ucount_max[type] = max <= LONG_MAX ? max : LONG_MAX;
> + ns->rlimit_max[type] = max <= LONG_MAX ? max : LONG_MAX;
> }
>
> #ifdef CONFIG_USER_NS
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 3244cc56b697..fb68d56bcd7e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -836,13 +836,13 @@ void __init fork_init(void)
> init_task.signal->rlim[RLIMIT_SIGPENDING] =
> init_task.signal->rlim[RLIMIT_NPROC];
>
> - for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++)
> + for (i = 0; i < UCOUNT_COUNTS; i++)
> init_user_ns.ucount_max[i] = max_threads/2;
>
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
> - set_rlimit_ucount_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_NPROC, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MSGQUEUE, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_SIGPENDING, RLIM_INFINITY);
> + set_userns_rlimit_max(&init_user_ns, UCOUNT_RLIMIT_MEMLOCK, RLIM_INFINITY);
>
> #ifdef CONFIG_VMAP_STACK
> cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "fork:vm_stack_cache",
> @@ -2053,7 +2053,7 @@ static __latent_entropy struct task_struct *copy_process(
> DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
> #endif
> retval = -EAGAIN;
> - if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> + if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> if (p->real_cred->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_free;
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8fdac0d90504..2c86d245acd6 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -479,7 +479,7 @@ static int set_user(struct cred *new)
> * for programs doing set*uid()+execve() by harmlessly deferring the
> * failure to the execve() stage.
> */
> - if (is_ucounts_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
> + if (is_rlimit_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
> new_user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> current->flags |= PF_NPROC_EXCEEDED;
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index 7b32c356ebc5..22070f004e97 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -87,10 +87,6 @@ static struct ctl_table user_table[] = {
> UCOUNT_ENTRY("max_fanotify_groups"),
> UCOUNT_ENTRY("max_fanotify_marks"),
> #endif
> - { },
> - { },
> - { },
> - { },
> { }
> };
> #endif /* CONFIG_SYSCTL */
> @@ -261,7 +257,7 @@ void dec_ucount(struct ucounts *ucounts, enum ucount_type type)
> put_ucounts(ucounts);
> }
>
> -long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
> +long inc_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v)
> {
> struct ucounts *iter;
> long max = LONG_MAX;
> @@ -273,12 +269,12 @@ long inc_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
> ret = LONG_MAX;
> else if (iter == ucounts)
> ret = new;
> - max = READ_ONCE(iter->ns->ucount_max[type]);
> + max = get_userns_rlimit_max(iter->ns, type);
> }
> return ret;
> }
>
> -bool dec_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
> +bool dec_rlimit_ucounts(struct ucounts *ucounts, enum rlimit_type type, long v)
> {
> struct ucounts *iter;
> long new = -1; /* Silence compiler warning */
> @@ -292,7 +288,7 @@ bool dec_rlimit_ucounts(struct ucounts *ucounts, enum ucount_type type, long v)
> }
>
> static void do_dec_rlimit_put_ucounts(struct ucounts *ucounts,
> - struct ucounts *last, enum ucount_type type)
> + struct ucounts *last, enum rlimit_type type)
> {
> struct ucounts *iter, *next;
> for (iter = ucounts; iter != last; iter = next) {
> @@ -304,12 +300,12 @@ static void do_dec_rlimit_put_ucounts(struct ucounts *ucounts,
> }
> }
>
> -void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum ucount_type type)
> +void dec_rlimit_put_ucounts(struct ucounts *ucounts, enum rlimit_type type)
> {
> do_dec_rlimit_put_ucounts(ucounts, NULL, type);
> }
>
> -long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
> +long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type)
> {
> /* Caller must hold a reference to ucounts */
> struct ucounts *iter;
> @@ -322,7 +318,7 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
> goto unwind;
> if (iter == ucounts)
> ret = new;
> - max = READ_ONCE(iter->ns->ucount_max[type]);
> + max = get_userns_rlimit_max(iter->ns, type);
> /*
> * Grab an extra ucount reference for the caller when
> * the rlimit count was previously 0.
> @@ -341,16 +337,16 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum ucount_type type)
> return 0;
> }
>
> -bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long rlimit)
> +bool is_rlimit_overlimit(struct ucounts *ucounts, enum rlimit_type type, unsigned long rlimit)
> {
> struct ucounts *iter;
> long max = rlimit;
> if (rlimit > LONG_MAX)
> max = LONG_MAX;
> for (iter = ucounts; iter; iter = iter->ns->ucounts) {
> - if (get_ucounts_value(iter, type) > max)
> + if (get_rlimit_value(iter, type) > max)
> return true;
> - max = READ_ONCE(iter->ns->ucount_max[type]);
> + max = get_userns_rlimit_max(iter->ns, type);
> }
> return false;
> }
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 6b2e3ca7ee99..b9f6729b4e5f 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -119,13 +119,13 @@ int create_user_ns(struct cred *new)
> ns->owner = owner;
> ns->group = group;
> INIT_WORK(&ns->work, free_user_ns);
> - for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++) {
> + for (i = 0; i < UCOUNT_COUNTS; i++) {
> ns->ucount_max[i] = INT_MAX;
> }
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
> - set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
> + set_userns_rlimit_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
> ns->ucounts = ucounts;
>
> /* Inherit USERNS_SETGROUPS_ALLOWED from our parent */