* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-14 11:12 UTC (permalink / raw)
To: Sagi Maimon, richardcochran, luto, datglx, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, maimon.sagi, keescook, legion,
mark.rutland, mszeredi, casey, reibax, davem, brauner
Cc: linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <20240314090540.14091-1-maimon.sagi@gmail.com>
On Thu, Mar 14 2024 at 11:05, Sagi Maimon wrote:
> Some user space applications need to read a couple of different clocks.
> Each read requires moving from user space to kernel space.
> Reading each clock separately (syscall) introduces extra
> unpredictable/unmeasurable delay. Minimizing this delay contributes to user
> space actions on these clocks (e.g. synchronization etc).
I asked for a proper description of the actual problem several times now
and you still provide some handwaving blurb. Feel free to ignore me, but
then please don't be surprised if I ignore you too.
Also why does reading two random clocks make any sense at all? Your code
allows to read CLOCK_TAI and CLOCK_THREAD_CPUTIME_ID. What for?
This is about PTP, no?
Again you completely fail to explain why the existing PTP ioctls are not
sufficient for the purpose and why you need to provide a new interface
which is completely ill defined.
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> drivers/ptp/ptp_clock.c | 34 ++++--
> include/linux/posix-clock.h | 2 +
> include/linux/syscalls.h | 4 +
> include/uapi/asm-generic/unistd.h | 5 +-
> kernel/time/posix-clock.c | 25 +++++
> kernel/time/posix-timers.c | 138 +++++++++++++++++++++++++
> kernel/time/posix-timers.h | 2 +
This needs to be split up into:
1) Infrastructure in posix-timers.c
2) Wire up the syscall in x86
3) Infrastructure in posix-clock.c
4) Usage in ptp_clock.c
and not as a big lump of everything.
> +/**
> + * clock_compare - Get couple of clocks time stamps
So the name of the syscall suggest that it compares two clocks, but the
actual functionality is to read two clocks.
This does not make any sense at all. Naming matters.
> + * @clock_a: clock a ID
> + * @clock_b: clock b ID
> + * @tp_a: Pointer to a user space timespec64 for clock a storage
> + * @tp_b: Pointer to a user space timespec64 for clock b storage
> + *
> + * clock_compare gets time sample of two clocks.
> + * Supported clocks IDs: PHC, virtual PHC and various system clocks.
> + *
> + * In case of PHC that supports crosstimespec and the other clock is Monotonic raw
> + * or system time, crosstimespec will be used to synchronously capture
> + * system/device time stamp.
> + *
> + * In other cases: Read clock_a twice (before, and after reading clock_b) and
> + * average these times – to be as close as possible to the time we read clock_b.
> + *
> + * Returns:
> + * 0 Success. @tp_a and @tp_b contains the time stamps
> + * -EINVAL @clock a or b ID is not a valid clock ID
> + * -EFAULT Copying the time stamp to @tp_a or @tp_b faulted
> + * -EOPNOTSUPP Dynamic POSIX clock does not support crosstimespec()
> + **/
> +SYSCALL_DEFINE5(clock_compare, const clockid_t, clock_a, const clockid_t, clock_b,
> + struct __kernel_timespec __user *, tp_a, struct __kernel_timespec __user *,
> + tp_b, s64 __user *, offs_err)
> +{
> + struct timespec64 ts_a, ts_a1, ts_b, ts_a2;
> + struct system_device_crosststamp xtstamp_a1, xtstamp_a2, xtstamp_b;
> + const struct k_clock *kc_a, *kc_b;
> + ktime_t ktime_a;
> + s64 ts_offs_err = 0;
> + int error = 0;
> + bool crosstime_support_a = false;
> + bool crosstime_support_b = false;
Please read and follow the documentation provided at:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> + kc_a = clockid_to_kclock(clock_a);
> + if (!kc_a) {
> + error = -EINVAL;
> + return error;
What's wrong about 'return -EINVAL;' ?
> + }
> +
> + kc_b = clockid_to_kclock(clock_b);
> + if (!kc_b) {
> + error = -EINVAL;
> + return error;
> + }
> +
> + // In case crosstimespec supported and b clock is Monotonic raw or system
> + // time, synchronously capture system/device time stamp
Please don't use C++ comments.
> + if (clock_a < 0) {
This is just wrong. posix-clocks ar not the only ones which have a
negative clock id. See clockid_to_kclock()
> + error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a1);
What's a crosstimespec?
This function fills in a system_device_crosststamp, so why not name it
so that the purpose of the function is obvious?
ptp_clock::info::getcrosststamp() has a reasonable name. So why do you
need to come up with something which makes the code obfuscated?
> + if (!error) {
> + if (clock_b == CLOCK_MONOTONIC_RAW) {
> + ts_b = ktime_to_timespec64(xtstamp_a1.sys_monoraw);
> + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
> + goto out;
> + } else if (clock_b == CLOCK_REALTIME) {
> + ts_b = ktime_to_timespec64(xtstamp_a1.sys_realtime);
> + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
> + goto out;
> + } else {
> + crosstime_support_a = true;
Right. If clock_b is anything else than CLOCK_MONOTONIC_RAW or
CLOCK_REALTIME then this is true.
> + }
> + }
So in case of an error, this just keeps going with an uninitialized
xtstamp_a1 and if the clock_b part succeeds it continues and operates on
garbage.
> + }
> +
> + // In case crosstimespec supported and a clock is Monotonic raw or system
> + // time, synchronously capture system/device time stamp
> + if (clock_b < 0) {
> + // Synchronously capture system/device time stamp
> + error = kc_b->clock_get_crosstimespec(clock_b, &xtstamp_b);
> + if (!error) {
> + if (clock_a == CLOCK_MONOTONIC_RAW) {
> + ts_a1 = ktime_to_timespec64(xtstamp_b.sys_monoraw);
> + ts_b = ktime_to_timespec64(xtstamp_b.device);
> + goto out;
> + } else if (clock_a == CLOCK_REALTIME) {
> + ts_a1 = ktime_to_timespec64(xtstamp_b.sys_realtime);
> + ts_b = ktime_to_timespec64(xtstamp_b.device);
> + goto out;
> + } else {
> + crosstime_support_b = true;
> + }
> + }
> + }
> +
> + if (crosstime_support_a)
> + error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a1);
What? crosstime_support_a is only true when the exactly same call
returned success. Why does it need to be called here again?
> + else
> + error = kc_a->clock_get_timespec(clock_a, &ts_a1);
> +
> + if (error)
> + return error;
> +
> + if (crosstime_support_b)
> + error = kc_b->clock_get_crosstimespec(clock_b, &xtstamp_b);
> + else
> + error = kc_b->clock_get_timespec(clock_b, &ts_b);
> +
> + if (error)
> + return error;
> +
> + if (crosstime_support_a)
> + error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a2);
> + else
> + error = kc_a->clock_get_timespec(clock_a, &ts_a2);
> +
> + if (error)
> + return error;
The logic and the code flow here are unreadable garbage and there are
zero comments what this is supposed to do.
> + if (crosstime_support_a) {
> + ktime_a = ktime_sub(xtstamp_a2.device, xtstamp_a1.device);
> + ts_offs_err = ktime_divns(ktime_a, 2);
> + ktime_a = ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_err);
> + ts_a1 = ktime_to_timespec64(ktime_a);
This is just wrong.
read(a1);
read(b);
read(a2);
You _CANNOT_ assume that (a1 + ((a2 - a1) / 2) is anywhere close to the
point in time where 'b' is read. This code is preemtible and
interruptible. I explained this to you before.
Your explanation in the comment above the function is just wishful
thinking.
> + * In other cases: Read clock_a twice (before, and after reading clock_b) and
> + * average these times – to be as close as possible to the time we read clock_b.
Can you please sit down and provide a precise technical description of
the problem you are trying to solve and explain your proposed solution
at the conceptual level instead of throwing out random implementations
every few days?
Thanks,
tglx
^ permalink raw reply
* [PATCH v7] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-03-14 9:05 UTC (permalink / raw)
To: richardcochran, luto, datglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, maimon.sagi, keescook, legion, mark.rutland,
mszeredi, casey, reibax, davem, brauner
Cc: linux-kernel, linux-api, linux-arch, netdev
Some user space applications need to read a couple of different clocks.
Each read requires moving from user space to kernel space.
Reading each clock separately (syscall) introduces extra
unpredictable/unmeasurable delay. Minimizing this delay contributes to user
space actions on these clocks (e.g. synchronization etc).
Introduce a new system call clock_compare, which can be used to measure
the offset between two clocks, from variety of types: PHC, virtual PHC
and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
The system call returns the clocks timestamps.
When possible, use crosstimespec to sync read values.
Else, read clock A twice (before, and after reading clock B) and average these
times – to be as close as possible to the time we read clock B.
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
Addressed comments from:
- Arnd Bergman : https://www.spinics.net/lists/netdev/msg980859.html
Changes since version 6:
- cheaper implantation regarding timespec64 operations.
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
drivers/ptp/ptp_clock.c | 34 ++++--
include/linux/posix-clock.h | 2 +
include/linux/syscalls.h | 4 +
include/uapi/asm-generic/unistd.h | 5 +-
kernel/time/posix-clock.c | 25 +++++
kernel/time/posix-timers.c | 138 +++++++++++++++++++++++++
kernel/time/posix-timers.h | 2 +
8 files changed, 200 insertions(+), 11 deletions(-)
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 7e8d46f4147f..727930d27e05 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -383,6 +383,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common clock_compare sys_clock_compare
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 15b804ba4868..37ce66d4159f 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -156,17 +156,31 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
return err;
}
+static int ptp_clock_getcrosstime(struct posix_clock *pc, struct system_device_crosststamp *xtstamp)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err;
+
+ if (!ptp->info->getcrosststamp)
+ err = -EOPNOTSUPP;
+ else
+ err = ptp->info->getcrosststamp(ptp->info, xtstamp);
+
+ return err;
+}
+
static struct posix_clock_operations ptp_clock_ops = {
- .owner = THIS_MODULE,
- .clock_adjtime = ptp_clock_adjtime,
- .clock_gettime = ptp_clock_gettime,
- .clock_getres = ptp_clock_getres,
- .clock_settime = ptp_clock_settime,
- .ioctl = ptp_ioctl,
- .open = ptp_open,
- .release = ptp_release,
- .poll = ptp_poll,
- .read = ptp_read,
+ .owner = THIS_MODULE,
+ .clock_adjtime = ptp_clock_adjtime,
+ .clock_gettime = ptp_clock_gettime,
+ .clock_getres = ptp_clock_getres,
+ .clock_settime = ptp_clock_settime,
+ .clock_getcrosstime = ptp_clock_getcrosstime,
+ .ioctl = ptp_ioctl,
+ .open = ptp_open,
+ .release = ptp_release,
+ .poll = ptp_poll,
+ .read = ptp_read,
};
static void ptp_clock_release(struct device *dev)
diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
index ef8619f48920..3a5b4bb3f56b 100644
--- a/include/linux/posix-clock.h
+++ b/include/linux/posix-clock.h
@@ -47,6 +47,8 @@ struct posix_clock_operations {
int (*clock_settime)(struct posix_clock *pc,
const struct timespec64 *ts);
+ int (*clock_getcrosstime)(struct posix_clock *pc,
+ struct system_device_crosststamp *xtstamp);
/*
* Optional character device methods:
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 77eb9b0e7685..47c5de3bdb18 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1188,6 +1188,10 @@ asmlinkage long sys_ni_syscall(void);
asmlinkage long sys_ni_posix_timers(void);
+asmlinkage long clock_compare(const clockid_t clock_a, const clockid_t clock_b,
+ struct __kernel_timespec __user *tp_a,
+ struct __kernel_timespec __user *tp_b,
+ s64 __user *offs_err);
/*
* Kernel code should not call syscalls (i.e., sys_xyzyyz()) directly.
* Instead, use one of the functions which work equivalently, such as
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 75f00965ab15..537a35afd237 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,11 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+#define __NR_clock_compare 462
+__SYSCALL(__NR_clock_compare, sys_clock_compare)
+
#undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 463
/*
* 32 bit systems traditionally used different
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 9de66bbbb3d1..68b2d6741036 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -327,9 +327,34 @@ static int pc_clock_settime(clockid_t id, const struct timespec64 *ts)
return err;
}
+static int pc_clock_get_crosstime(clockid_t id, struct system_device_crosststamp *xtstamp)
+{
+ struct posix_clock_desc cd;
+ int err;
+
+ err = get_clock_desc(id, &cd);
+ if (err)
+ return err;
+
+ if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
+ err = -EACCES;
+ goto out;
+ }
+
+ if (cd.clk->ops.clock_getcrosstime)
+ err = cd.clk->ops.clock_getcrosstime(cd.clk, xtstamp);
+ else
+ err = -EOPNOTSUPP;
+out:
+ put_clock_desc(&cd);
+
+ return err;
+}
+
const struct k_clock clock_posix_dynamic = {
.clock_getres = pc_clock_getres,
.clock_set = pc_clock_settime,
.clock_get_timespec = pc_clock_gettime,
.clock_adj = pc_clock_adjtime,
+ .clock_get_crosstimespec = pc_clock_get_crosstime,
};
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index b924f0f096fa..ea43527cd5e9 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1426,6 +1426,144 @@ SYSCALL_DEFINE4(clock_nanosleep_time32, clockid_t, which_clock, int, flags,
#endif
+/**
+ * clock_compare - Get couple of clocks time stamps
+ * @clock_a: clock a ID
+ * @clock_b: clock b ID
+ * @tp_a: Pointer to a user space timespec64 for clock a storage
+ * @tp_b: Pointer to a user space timespec64 for clock b storage
+ *
+ * clock_compare gets time sample of two clocks.
+ * Supported clocks IDs: PHC, virtual PHC and various system clocks.
+ *
+ * In case of PHC that supports crosstimespec and the other clock is Monotonic raw
+ * or system time, crosstimespec will be used to synchronously capture
+ * system/device time stamp.
+ *
+ * In other cases: Read clock_a twice (before, and after reading clock_b) and
+ * average these times – to be as close as possible to the time we read clock_b.
+ *
+ * Returns:
+ * 0 Success. @tp_a and @tp_b contains the time stamps
+ * -EINVAL @clock a or b ID is not a valid clock ID
+ * -EFAULT Copying the time stamp to @tp_a or @tp_b faulted
+ * -EOPNOTSUPP Dynamic POSIX clock does not support crosstimespec()
+ **/
+SYSCALL_DEFINE5(clock_compare, const clockid_t, clock_a, const clockid_t, clock_b,
+ struct __kernel_timespec __user *, tp_a, struct __kernel_timespec __user *,
+ tp_b, s64 __user *, offs_err)
+{
+ struct timespec64 ts_a, ts_a1, ts_b, ts_a2;
+ struct system_device_crosststamp xtstamp_a1, xtstamp_a2, xtstamp_b;
+ const struct k_clock *kc_a, *kc_b;
+ ktime_t ktime_a;
+ s64 ts_offs_err = 0;
+ int error = 0;
+ bool crosstime_support_a = false;
+ bool crosstime_support_b = false;
+
+ kc_a = clockid_to_kclock(clock_a);
+ if (!kc_a) {
+ error = -EINVAL;
+ return error;
+ }
+
+ kc_b = clockid_to_kclock(clock_b);
+ if (!kc_b) {
+ error = -EINVAL;
+ return error;
+ }
+
+ // In case crosstimespec supported and b clock is Monotonic raw or system
+ // time, synchronously capture system/device time stamp
+ if (clock_a < 0) {
+ error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a1);
+ if (!error) {
+ if (clock_b == CLOCK_MONOTONIC_RAW) {
+ ts_b = ktime_to_timespec64(xtstamp_a1.sys_monoraw);
+ ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
+ goto out;
+ } else if (clock_b == CLOCK_REALTIME) {
+ ts_b = ktime_to_timespec64(xtstamp_a1.sys_realtime);
+ ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
+ goto out;
+ } else {
+ crosstime_support_a = true;
+ }
+ }
+ }
+
+ // In case crosstimespec supported and a clock is Monotonic raw or system
+ // time, synchronously capture system/device time stamp
+ if (clock_b < 0) {
+ // Synchronously capture system/device time stamp
+ error = kc_b->clock_get_crosstimespec(clock_b, &xtstamp_b);
+ if (!error) {
+ if (clock_a == CLOCK_MONOTONIC_RAW) {
+ ts_a1 = ktime_to_timespec64(xtstamp_b.sys_monoraw);
+ ts_b = ktime_to_timespec64(xtstamp_b.device);
+ goto out;
+ } else if (clock_a == CLOCK_REALTIME) {
+ ts_a1 = ktime_to_timespec64(xtstamp_b.sys_realtime);
+ ts_b = ktime_to_timespec64(xtstamp_b.device);
+ goto out;
+ } else {
+ crosstime_support_b = true;
+ }
+ }
+ }
+
+ if (crosstime_support_a)
+ error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a1);
+ else
+ error = kc_a->clock_get_timespec(clock_a, &ts_a1);
+
+ if (error)
+ return error;
+
+ if (crosstime_support_b)
+ error = kc_b->clock_get_crosstimespec(clock_b, &xtstamp_b);
+ else
+ error = kc_b->clock_get_timespec(clock_b, &ts_b);
+
+ if (error)
+ return error;
+
+ if (crosstime_support_a)
+ error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a2);
+ else
+ error = kc_a->clock_get_timespec(clock_a, &ts_a2);
+
+ if (error)
+ return error;
+
+ if (crosstime_support_a) {
+ ktime_a = ktime_sub(xtstamp_a2.device, xtstamp_a1.device);
+ ts_offs_err = ktime_divns(ktime_a, 2);
+ ktime_a = ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_err);
+ ts_a1 = ktime_to_timespec64(ktime_a);
+ } else {
+ ts_a = timespec64_sub(ts_a2, ts_a1);
+ ktime_a = timespec64_to_ktime(ts_a);
+ ts_offs_err = ktime_divns(ktime_a, 2);
+ timespec64_add_ns(&ts_a1, (u64)ts_offs_err);
+ }
+
+ if (crosstime_support_b)
+ ts_b = ktime_to_timespec64(xtstamp_b.device);
+out:
+ if (put_timespec64(&ts_a1, tp_a))
+ error = -EFAULT;
+
+ if (!error && put_timespec64(&ts_b, tp_b))
+ error = -EFAULT;
+
+ if (!error && copy_to_user(offs_err, &ts_offs_err, sizeof(ts_offs_err)))
+ error = -EFAULT;
+
+ return error;
+}
+
static const struct k_clock clock_realtime = {
.clock_getres = posix_get_hrtimer_res,
.clock_get_timespec = posix_get_realtime_timespec,
diff --git a/kernel/time/posix-timers.h b/kernel/time/posix-timers.h
index f32a2ebba9b8..b1f6075f35bb 100644
--- a/kernel/time/posix-timers.h
+++ b/kernel/time/posix-timers.h
@@ -11,6 +11,8 @@ struct k_clock {
struct timespec64 *tp);
/* Returns the clock value in the root time namespace. */
ktime_t (*clock_get_ktime)(const clockid_t which_clock);
+ int (*clock_get_crosstimespec)(const clockid_t which_clock,
+ struct system_device_crosststamp *xtstamp);
int (*clock_adj)(const clockid_t which_clock, struct __kernel_timex *tx);
int (*timer_create)(struct k_itimer *timer);
int (*nsleep)(const clockid_t which_clock, int flags,
--
2.26.3
^ permalink raw reply related
* Re: [PATCH v6] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-03-14 8:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Richard Cochran, Andy Lutomirski, datglx, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Geert Uytterhoeven, Peter Zijlstra, Johannes Weiner, Sohil Mehta,
Rick Edgecombe, Nhat Pham, Palmer Dabbelt, Kees Cook,
Alexey Gladkov, Mark Rutland, Miklos Szeredi, Casey Schaufler,
reibax, David S . Miller, Christian Brauner, linux-kernel,
linux-api, Linux-Arch, Netdev
In-Reply-To: <0a4e4505-cf04-4481-955c-1e35cf97ff8d@app.fastmail.com>
On Tue, Mar 12, 2024 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Mar 12, 2024, at 13:15, Sagi Maimon wrote:
> > On Tue, Mar 12, 2024 at 1:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Tue, Mar 12, 2024, at 10:50, Sagi Maimon wrote:
> >> > + kc_a = clockid_to_kclock(clock_a);
> >> > + if (!kc_a) {
> >> > + error = -EINVAL;
> >> > + return error;
> >> > + }
> >> > +
> >> > + kc_b = clockid_to_kclock(clock_b);
> >> > + if (!kc_b) {
> >> > + error = -EINVAL;
> >> > + return error;
> >> > + }
> >>
> >> I'm not sure if we really need to have it generic enough to
> >> support any combination of clocks here. It complicates the
> >> implementation a bit but it also generalizes the user space
> >> side of it.
> >>
> >> Can you think of cases where you want to compare against
> >> something other than CLOCK_MONOTONIC_RAW or CLOCK_REALTIME,
> >> or are these going to be the ones that you expect to
> >> be used anyway?
> >>
> > sure, one example is syncing two different PHCs (which was originally
> > why we needed this syscall)
> > I hope that I have understand your note and that answers your question.
>
> Right, that is clearly a sensible use case.
>
> I'm still trying to understand the implementation for the case
> where you have two different PHCs and both implement
> clock_get_crosstimespec(). Rather than averaging between
> two snapshots here, I would expect this to result in
> something like
>
> ktime_a1 += xtstamp_b.sys_monoraw - xtstamp_a1.sys_monoraw;
>
> in order get two device timestamps ktime_a1 and ktime_b
> that reflect the snapshots as if they were taken
> simulatenously. Am I missing some finer detail here,
> or is this something you should do?
>
Since the raw monotonic clock and the PHC are not synthesized, that
won't be accurate at all and depends on the frequency delta between
them.
> >> > + if (crosstime_support_a) {
> >> > + ktime_a1 = xtstamp_a1.device;
> >> > + ktime_a2 = xtstamp_a2.device;
> >> > + } else {
> >> > + ktime_a1 = timespec64_to_ktime(ts_a1);
> >> > + ktime_a2 = timespec64_to_ktime(ts_a2);
> >> > + }
> >> > +
> >> > + ktime_a = ktime_add(ktime_a1, ktime_a2);
> >> > +
> >> > + ts_offs = ktime_divns(ktime_a, 2);
> >> > +
> >> > + ts_a1 = ns_to_timespec64(ts_offs);
> >>
> >> Converting nanoseconds to timespec64 is rather expensive,
> >> so I wonder if this could be changed to something cheaper,
> >> either by returning nanoseconds in the end and consistently
> >> working on those, or by doing the calculation on the
> >> timespec64 itself.
> >>
> > I prefer returning timespec64, so this system call aligns with other
> > system calls like clock_gettime for example.
> > As far as doing the calculation on timespec64 itself, that looks more
> > expansive to me, but I might be wrong.
>
> In the general case, dividing a 64-bit variable by some other
> variable is really expensive and will take hundreds of cycles.
> This one is a bit cheaper because the division is done using
> a constant divider of NS_PER_SEC, which can get optimized fairly
> well on many systems by turning it into an equivalent 128-bit
> multiplication plus shift.
>
> For the case where you start out with a timespec64, I would
> expect it to be cheaper to calculate the nanosecond difference
> between ts_a1 and ts_a2 to add half of that to the timespec
> than to average two large 64-bit values and convert that back
> to a timespec afterwards. This should be fairly easy to try
> out if you can test a 32-bit kernel. We could decide that
> there is no need to care about anything bug 64-bit kernels
> here, in which case your current version should be just as
> good for both the crosstime_support_a and !crosstime_support_a
> cases.
>
sounds good, it will be done on the next patch.
> Arnd
^ permalink raw reply
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Paul Moore @ 2024-03-14 2:25 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dmitry V. Levin, LSM List, Linux kernel mailing list, linux-api,
Mickaël Salaün, James Morris, Serge Hallyn,
John Johansen, Tetsuo Handa, Stephen Smalley
In-Reply-To: <CAHC9VhQyje1KdbXLKhZ3atgDbf2mNHB409BHtNyE_RSBACpB7g@mail.gmail.com>
On Wed, Mar 13, 2024 at 9:44 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Mar 13, 2024 at 6:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 3/13/2024 3:37 PM, Paul Moore wrote:
> > > On Wed, Mar 13, 2024 at 4:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > >> On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>> LSM: use 32 bit compatible data types in LSM syscalls.
> > >>>
> > >>> Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
> > >>> and lsm_get_self_attr() from size_t to u32. This avoids the need to
> > >>> have different interfaces for 32 and 64 bit systems.
> > >>>
> > >>> Cc: stable@vger.kernel.org
> > >>> Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
> > >>> Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
> > >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > >>> Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
> > >>> ---
> > >>> include/linux/lsm_hook_defs.h | 4 ++--
> > >>> include/linux/security.h | 8 ++++----
> > >>> security/apparmor/lsm.c | 4 ++--
> > >>> security/lsm_syscalls.c | 10 +++++-----
> > >>> security/security.c | 12 ++++++------
> > >>> security/selinux/hooks.c | 4 ++--
> > >>> security/smack/smack_lsm.c | 4 ++--
> > >>> tools/testing/selftests/lsm/common.h | 6 +++---
> > >>> tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
> > >>> tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
> > >>> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
> > >>> 11 files changed, 38 insertions(+), 38 deletions(-)
> > >> Okay, this looks better, I'm going to merge this into lsm/stable-6.9
> > >> and put it through the usual automated testing as well as a kselftest
> > >> run to make sure everything there is still okay. Assuming all goes
> > >> well and no one raises any objections, I'll likely send this up to
> > >> Linus tomorrow.
> > >>
> > >> Thanks everyone!
> > >
> > > Unfortunately it looks like we have a kselftest failure (below). I'm
> > > pretty sure that this was working at some point, but it's possible I
> > > missed it when I ran the selftests previously. I've got to break for
> > > a personal appt right now, but I'll dig into this later tonight.
> >
> > In v2:
> >
> > diff --git a/security/security.c b/security/security.c
> > index 7035ee35a393..a0f9caf89ae1 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> > nctx->ctx_len = val_len;
> > memcpy(nctx->ctx, val, val_len);
> >
> > - if (copy_to_user(uctx, nctx, nctx_len))
> > + if (uctx && copy_to_user(uctx, nctx, nctx_len))
> > rc = -EFAULT;
> >
> > out:
> >
> > This addresses the case where NULL is passed in the call to lsm_get_self_attr()
> > to get the buffer size required.
>
> Yeah, thanks. I didn't get a chance to look at the failure before I
> had to leave, but now that I'm looking at it I agree. It looks like
> it used to work prior to d7cf3412a9f6c, but I broke things when I
> consolidated the processing into lsm_fill_user_ctx() - oops :/
>
> I'll start working on the patch right now and post it as soon as it
> passes testing.
The patch posted below passes the kselftests and all my other sanity checks:
https://lore.kernel.org/linux-security-module/20240314022202.599471-2-paul@paul-moore.com
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Paul Moore @ 2024-03-14 1:44 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dmitry V. Levin, LSM List, Linux kernel mailing list, linux-api,
Mickaël Salaün, James Morris, Serge Hallyn,
John Johansen, Tetsuo Handa, Stephen Smalley
In-Reply-To: <b5ebbb40-0dda-4595-a058-d5c3a6e800df@schaufler-ca.com>
On Wed, Mar 13, 2024 at 6:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/13/2024 3:37 PM, Paul Moore wrote:
> > On Wed, Mar 13, 2024 at 4:07 PM Paul Moore <paul@paul-moore.com> wrote:
> >> On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> LSM: use 32 bit compatible data types in LSM syscalls.
> >>>
> >>> Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
> >>> and lsm_get_self_attr() from size_t to u32. This avoids the need to
> >>> have different interfaces for 32 and 64 bit systems.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
> >>> Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>> Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
> >>> ---
> >>> include/linux/lsm_hook_defs.h | 4 ++--
> >>> include/linux/security.h | 8 ++++----
> >>> security/apparmor/lsm.c | 4 ++--
> >>> security/lsm_syscalls.c | 10 +++++-----
> >>> security/security.c | 12 ++++++------
> >>> security/selinux/hooks.c | 4 ++--
> >>> security/smack/smack_lsm.c | 4 ++--
> >>> tools/testing/selftests/lsm/common.h | 6 +++---
> >>> tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
> >>> tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
> >>> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
> >>> 11 files changed, 38 insertions(+), 38 deletions(-)
> >> Okay, this looks better, I'm going to merge this into lsm/stable-6.9
> >> and put it through the usual automated testing as well as a kselftest
> >> run to make sure everything there is still okay. Assuming all goes
> >> well and no one raises any objections, I'll likely send this up to
> >> Linus tomorrow.
> >>
> >> Thanks everyone!
> >
> > Unfortunately it looks like we have a kselftest failure (below). I'm
> > pretty sure that this was working at some point, but it's possible I
> > missed it when I ran the selftests previously. I've got to break for
> > a personal appt right now, but I'll dig into this later tonight.
>
> In v2:
>
> diff --git a/security/security.c b/security/security.c
> index 7035ee35a393..a0f9caf89ae1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> nctx->ctx_len = val_len;
> memcpy(nctx->ctx, val, val_len);
>
> - if (copy_to_user(uctx, nctx, nctx_len))
> + if (uctx && copy_to_user(uctx, nctx, nctx_len))
> rc = -EFAULT;
>
> out:
>
> This addresses the case where NULL is passed in the call to lsm_get_self_attr()
> to get the buffer size required.
Yeah, thanks. I didn't get a chance to look at the failure before I
had to leave, but now that I'm looking at it I agree. It looks like
it used to work prior to d7cf3412a9f6c, but I broke things when I
consolidated the processing into lsm_fill_user_ctx() - oops :/
I'll start working on the patch right now and post it as soon as it
passes testing.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Casey Schaufler @ 2024-03-13 22:48 UTC (permalink / raw)
To: Paul Moore, Dmitry V. Levin, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <CAHC9VhTkvyWpvkejbFf-VJoTvUKVDGxBDYkKFdNrdgq4jy5i_w@mail.gmail.com>
On 3/13/2024 3:37 PM, Paul Moore wrote:
> On Wed, Mar 13, 2024 at 4:07 PM Paul Moore <paul@paul-moore.com> wrote:
>> On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> LSM: use 32 bit compatible data types in LSM syscalls.
>>>
>>> Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
>>> and lsm_get_self_attr() from size_t to u32. This avoids the need to
>>> have different interfaces for 32 and 64 bit systems.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
>>> Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
>>> ---
>>> include/linux/lsm_hook_defs.h | 4 ++--
>>> include/linux/security.h | 8 ++++----
>>> security/apparmor/lsm.c | 4 ++--
>>> security/lsm_syscalls.c | 10 +++++-----
>>> security/security.c | 12 ++++++------
>>> security/selinux/hooks.c | 4 ++--
>>> security/smack/smack_lsm.c | 4 ++--
>>> tools/testing/selftests/lsm/common.h | 6 +++---
>>> tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
>>> tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
>>> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
>>> 11 files changed, 38 insertions(+), 38 deletions(-)
>> Okay, this looks better, I'm going to merge this into lsm/stable-6.9
>> and put it through the usual automated testing as well as a kselftest
>> run to make sure everything there is still okay. Assuming all goes
>> well and no one raises any objections, I'll likely send this up to
>> Linus tomorrow.
>>
>> Thanks everyone!
> Unfortunately it looks like we have a kselftest failure (below). I'm
> pretty sure that this was working at some point, but it's possible I
> missed it when I ran the selftests previously. I've got to break for
> a personal appt right now, but I'll dig into this later tonight.
In v2:
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..a0f9caf89ae1 100644
--- a/security/security.c
+++ b/security/security.c
@@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
nctx->ctx_len = val_len;
memcpy(nctx->ctx, val, val_len);
- if (copy_to_user(uctx, nctx, nctx_len))
+ if (uctx && copy_to_user(uctx, nctx, nctx_len))
rc = -EFAULT;
out:
This addresses the case where NULL is passed in the call to lsm_get_self_attr()
to get the buffer size required.
>
> # timeout set to 45
> # selftests: lsm: lsm_get_self_attr_test
> # TAP version 13
> # 1..6
> # # Starting 6 tests from 1 test cases.
> # # RUN global.size_null_lsm_get_self_attr ...
> # # OK global.size_null_lsm_get_self_attr
> # ok 1 global.size_null_lsm_get_self_attr
> # # RUN global.ctx_null_lsm_get_self_attr ...
> # # lsm_get_self_attr_test.c:49:ctx_null_lsm_get_self_attr:Expected -1 (-1) != r
> c (-1)
> # # ctx_null_lsm_get_self_attr: Test terminated by assertion
> # # FAIL global.ctx_null_lsm_get_self_attr
> # not ok 2 global.ctx_null_lsm_get_self_attr
> # # RUN global.size_too_small_lsm_get_self_attr ...
> # # OK global.size_too_small_lsm_get_self_attr
> # ok 3 global.size_too_small_lsm_get_self_attr
> # # RUN global.flags_zero_lsm_get_self_attr ...
> # # OK global.flags_zero_lsm_get_self_attr
> # ok 4 global.flags_zero_lsm_get_self_attr
> # # RUN global.flags_overset_lsm_get_self_attr ...
> # # OK global.flags_overset_lsm_get_self_attr
> # ok 5 global.flags_overset_lsm_get_self_attr
> # # RUN global.basic_lsm_get_self_attr ...
> # # OK global.basic_lsm_get_self_attr
> # ok 6 global.basic_lsm_get_self_attr
> # # FAILED: 5 / 6 tests passed.
> # # Totals: pass:5 fail:1 xfail:0 xpass:0 skip:0 error:0
> not ok 1 selftests: lsm: lsm_get_self_attr_test # exit=1
>
^ permalink raw reply related
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Paul Moore @ 2024-03-13 22:37 UTC (permalink / raw)
To: Casey Schaufler, Dmitry V. Levin, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley
In-Reply-To: <ef972e0088964722adffc596d38b0463@paul-moore.com>
On Wed, Mar 13, 2024 at 4:07 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > LSM: use 32 bit compatible data types in LSM syscalls.
> >
> > Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
> > and lsm_get_self_attr() from size_t to u32. This avoids the need to
> > have different interfaces for 32 and 64 bit systems.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
> > Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
> > ---
> > include/linux/lsm_hook_defs.h | 4 ++--
> > include/linux/security.h | 8 ++++----
> > security/apparmor/lsm.c | 4 ++--
> > security/lsm_syscalls.c | 10 +++++-----
> > security/security.c | 12 ++++++------
> > security/selinux/hooks.c | 4 ++--
> > security/smack/smack_lsm.c | 4 ++--
> > tools/testing/selftests/lsm/common.h | 6 +++---
> > tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
> > tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
> > tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
> > 11 files changed, 38 insertions(+), 38 deletions(-)
>
> Okay, this looks better, I'm going to merge this into lsm/stable-6.9
> and put it through the usual automated testing as well as a kselftest
> run to make sure everything there is still okay. Assuming all goes
> well and no one raises any objections, I'll likely send this up to
> Linus tomorrow.
>
> Thanks everyone!
Unfortunately it looks like we have a kselftest failure (below). I'm
pretty sure that this was working at some point, but it's possible I
missed it when I ran the selftests previously. I've got to break for
a personal appt right now, but I'll dig into this later tonight.
# timeout set to 45
# selftests: lsm: lsm_get_self_attr_test
# TAP version 13
# 1..6
# # Starting 6 tests from 1 test cases.
# # RUN global.size_null_lsm_get_self_attr ...
# # OK global.size_null_lsm_get_self_attr
# ok 1 global.size_null_lsm_get_self_attr
# # RUN global.ctx_null_lsm_get_self_attr ...
# # lsm_get_self_attr_test.c:49:ctx_null_lsm_get_self_attr:Expected -1 (-1) != r
c (-1)
# # ctx_null_lsm_get_self_attr: Test terminated by assertion
# # FAIL global.ctx_null_lsm_get_self_attr
# not ok 2 global.ctx_null_lsm_get_self_attr
# # RUN global.size_too_small_lsm_get_self_attr ...
# # OK global.size_too_small_lsm_get_self_attr
# ok 3 global.size_too_small_lsm_get_self_attr
# # RUN global.flags_zero_lsm_get_self_attr ...
# # OK global.flags_zero_lsm_get_self_attr
# ok 4 global.flags_zero_lsm_get_self_attr
# # RUN global.flags_overset_lsm_get_self_attr ...
# # OK global.flags_overset_lsm_get_self_attr
# ok 5 global.flags_overset_lsm_get_self_attr
# # RUN global.basic_lsm_get_self_attr ...
# # OK global.basic_lsm_get_self_attr
# ok 6 global.basic_lsm_get_self_attr
# # FAILED: 5 / 6 tests passed.
# # Totals: pass:5 fail:1 xfail:0 xpass:0 skip:0 error:0
not ok 1 selftests: lsm: lsm_get_self_attr_test # exit=1
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Paul Moore @ 2024-03-13 20:07 UTC (permalink / raw)
To: Casey Schaufler, Dmitry V. Levin, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <da4d181d-16b9-4e0f-a744-ac61702e0b63@schaufler-ca.com>
On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> LSM: use 32 bit compatible data types in LSM syscalls.
>
> Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
> and lsm_get_self_attr() from size_t to u32. This avoids the need to
> have different interfaces for 32 and 64 bit systems.
>
> Cc: stable@vger.kernel.org
> Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
> Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
> ---
> include/linux/lsm_hook_defs.h | 4 ++--
> include/linux/security.h | 8 ++++----
> security/apparmor/lsm.c | 4 ++--
> security/lsm_syscalls.c | 10 +++++-----
> security/security.c | 12 ++++++------
> security/selinux/hooks.c | 4 ++--
> security/smack/smack_lsm.c | 4 ++--
> tools/testing/selftests/lsm/common.h | 6 +++---
> tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
> tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
> 11 files changed, 38 insertions(+), 38 deletions(-)
Okay, this looks better, I'm going to merge this into lsm/stable-6.9
and put it through the usual automated testing as well as a kselftest
run to make sure everything there is still okay. Assuming all goes
well and no one raises any objections, I'll likely send this up to
Linus tomorrow.
Thanks everyone!
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Dmitry V. Levin @ 2024-03-13 19:42 UTC (permalink / raw)
To: Casey Schaufler, Paul Moore
Cc: LSM List, Linux kernel mailing list, linux-api,
Mickaël Salaün, James Morris, Serge Hallyn,
John Johansen, Tetsuo Handa, Stephen Smalley
In-Reply-To: <da4d181d-16b9-4e0f-a744-ac61702e0b63@schaufler-ca.com>
On Wed, Mar 13, 2024 at 12:32:37PM -0700, Casey Schaufler wrote:
> LSM: use 32 bit compatible data types in LSM syscalls.
>
> Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
> and lsm_get_self_attr() from size_t to u32. This avoids the need to
> have different interfaces for 32 and 64 bit systems.
>
> Cc: stable@vger.kernel.org
> Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
> Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
--
ldv
^ permalink raw reply
* [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Casey Schaufler @ 2024-03-13 19:32 UTC (permalink / raw)
To: Dmitry V. Levin, Paul Moore, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <00734a64-a5fe-420c-bf6e-bee27c9d83be.ref@schaufler-ca.com>
LSM: use 32 bit compatible data types in LSM syscalls.
Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
and lsm_get_self_attr() from size_t to u32. This avoids the need to
have different interfaces for 32 and 64 bit systems.
Cc: stable@vger.kernel.org
Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hook_defs.h | 4 ++--
include/linux/security.h | 8 ++++----
security/apparmor/lsm.c | 4 ++--
security/lsm_syscalls.c | 10 +++++-----
security/security.c | 12 ++++++------
security/selinux/hooks.c | 4 ++--
security/smack/smack_lsm.c | 4 ++--
tools/testing/selftests/lsm/common.h | 6 +++---
tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
11 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..f9b5baf1b5f4 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -265,9 +265,9 @@ LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
struct inode *inode)
LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
- struct lsm_ctx __user *ctx, size_t *size, u32 flags)
+ struct lsm_ctx __user *ctx, u32 *size, u32 flags)
LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
- struct lsm_ctx *ctx, size_t size, u32 flags)
+ struct lsm_ctx *ctx, u32 size, u32 flags)
LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
char **value)
LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..3180d823e023 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -478,9 +478,9 @@ int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
void security_d_instantiate(struct dentry *dentry, struct inode *inode);
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t __user *size, u32 flags);
+ u32 __user *size, u32 flags);
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t size, u32 flags);
+ u32 size, u32 flags);
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value);
int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
@@ -494,7 +494,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
-int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
void *val, size_t val_len, u64 id, u64 flags);
#else /* CONFIG_SECURITY */
@@ -1434,7 +1434,7 @@ static inline int security_locked_down(enum lockdown_reason what)
return 0;
}
static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
- size_t *uctx_len, void *val, size_t val_len,
+ u32 *uctx_len, void *val, size_t val_len,
u64 id, u64 flags)
{
return -EOPNOTSUPP;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 9a3dcaafb5b1..cef8c466af80 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -779,7 +779,7 @@ static int apparmor_sb_pivotroot(const struct path *old_path,
}
static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
- size_t *size, u32 flags)
+ u32 *size, u32 flags)
{
int error = -ENOENT;
struct aa_task_ctx *ctx = task_ctx(current);
@@ -924,7 +924,7 @@ static int do_setattr(u64 attr, void *value, size_t size)
}
static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
int rc;
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 5d391b1f7e69..8440948a690c 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -53,7 +53,7 @@ u64 lsm_name_to_attr(const char *name)
* value indicating the reason for the error is returned.
*/
SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
- ctx, size_t, size, u32, flags)
+ ctx, u32, size, u32, flags)
{
return security_setselfattr(attr, ctx, size, flags);
}
@@ -75,7 +75,7 @@ SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
* a negative value indicating the error is returned.
*/
SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
- ctx, size_t __user *, size, u32, flags)
+ ctx, u32 __user *, size, u32, flags)
{
return security_getselfattr(attr, ctx, size, flags);
}
@@ -93,11 +93,11 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
* required size. In all other cases a negative value indicating the
* error is returned.
*/
-SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
+SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size,
u32, flags)
{
- size_t total_size = lsm_active_cnt * sizeof(*ids);
- size_t usize;
+ u32 total_size = lsm_active_cnt * sizeof(*ids);
+ u32 usize;
int i;
if (flags)
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..fb7505c73485 100644
--- a/security/security.c
+++ b/security/security.c
@@ -785,7 +785,7 @@ static int lsm_superblock_alloc(struct super_block *sb)
* Returns 0 on success, -E2BIG if userspace buffer is not large enough,
* -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
*/
-int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
void *val, size_t val_len,
u64 id, u64 flags)
{
@@ -3918,14 +3918,14 @@ EXPORT_SYMBOL(security_d_instantiate);
* If @size is insufficient to contain the data -E2BIG is returned.
*/
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
- size_t __user *size, u32 flags)
+ u32 __user *size, u32 flags)
{
struct security_hook_list *hp;
struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
u8 __user *base = (u8 __user *)uctx;
- size_t total = 0;
- size_t entrysize;
- size_t left;
+ u32 entrysize;
+ u32 total = 0;
+ u32 left;
bool toobig = false;
bool single = false;
int count = 0;
@@ -4011,7 +4011,7 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
* LSM specific failure.
*/
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
struct security_hook_list *hp;
struct lsm_ctx *lctx;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 338b023a8c3e..71e6e7079d7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6556,7 +6556,7 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
* There will only ever be one attribute.
*/
static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t *size, u32 flags)
+ u32 *size, u32 flags)
{
int rc;
char *val = NULL;
@@ -6571,7 +6571,7 @@ static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
}
static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
int rc;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..0891fcc11e40 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3641,7 +3641,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
* There will only ever be one attribute.
*/
static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t *size, u32 flags)
+ u32 *size, u32 flags)
{
int rc;
struct smack_known *skp;
@@ -3762,7 +3762,7 @@ static int do_setattr(u64 attr, void *value, size_t size)
* Returns 0 on success, an error code otherwise.
*/
static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
int rc;
diff --git a/tools/testing/selftests/lsm/common.h b/tools/testing/selftests/lsm/common.h
index d404329e5eeb..06d12110d241 100644
--- a/tools/testing/selftests/lsm/common.h
+++ b/tools/testing/selftests/lsm/common.h
@@ -7,7 +7,7 @@
#ifndef lsm_get_self_attr
static inline int lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
- size_t *size, __u32 flags)
+ __u32 *size, __u32 flags)
{
return syscall(__NR_lsm_get_self_attr, attr, ctx, size, flags);
}
@@ -15,14 +15,14 @@ static inline int lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
#ifndef lsm_set_self_attr
static inline int lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, __u32 flags)
+ __u32 size, __u32 flags)
{
return syscall(__NR_lsm_set_self_attr, attr, ctx, size, flags);
}
#endif
#ifndef lsm_list_modules
-static inline int lsm_list_modules(__u64 *ids, size_t *size, __u32 flags)
+static inline int lsm_list_modules(__u64 *ids, __u32 *size, __u32 flags)
{
return syscall(__NR_lsm_list_modules, ids, size, flags);
}
diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
index e0e313d9047a..df215e4aa63f 100644
--- a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
+++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
@@ -40,7 +40,7 @@ TEST(size_null_lsm_get_self_attr)
TEST(ctx_null_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
int rc;
rc = lsm_get_self_attr(LSM_ATTR_CURRENT, NULL, &size, 0);
@@ -57,7 +57,7 @@ TEST(size_too_small_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = 1;
+ __u32 size = 1;
ASSERT_NE(NULL, ctx);
errno = 0;
@@ -77,7 +77,7 @@ TEST(flags_zero_lsm_get_self_attr)
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size;
+ __u32 size;
int lsmcount;
int i;
@@ -117,7 +117,7 @@ TEST(flags_overset_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size;
+ __u32 size;
ASSERT_NE(NULL, ctx);
@@ -140,7 +140,7 @@ TEST(flags_overset_lsm_get_self_attr)
TEST(basic_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
struct lsm_ctx *ctx = calloc(page_size, 1);
struct lsm_ctx *tctx = NULL;
__u64 *syscall_lsms = calloc(page_size, 1);
diff --git a/tools/testing/selftests/lsm/lsm_list_modules_test.c b/tools/testing/selftests/lsm/lsm_list_modules_test.c
index 9df29b1e3497..868641dbb309 100644
--- a/tools/testing/selftests/lsm/lsm_list_modules_test.c
+++ b/tools/testing/selftests/lsm/lsm_list_modules_test.c
@@ -31,7 +31,7 @@ TEST(size_null_lsm_list_modules)
TEST(ids_null_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
errno = 0;
ASSERT_EQ(-1, lsm_list_modules(NULL, &size, 0));
@@ -43,7 +43,7 @@ TEST(size_too_small_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size = 1;
+ __u32 size = 1;
ASSERT_NE(NULL, syscall_lsms);
errno = 0;
@@ -58,7 +58,7 @@ TEST(flags_set_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
ASSERT_NE(NULL, syscall_lsms);
errno = 0;
@@ -72,7 +72,7 @@ TEST(flags_set_lsm_list_modules)
TEST(correct_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
__u64 *syscall_lsms = calloc(page_size, 1);
char *sysfs_lsms = calloc(page_size, 1);
char *name;
diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
index e9712c6cf596..66dec47e3ca3 100644
--- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
+++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
@@ -25,7 +25,7 @@ TEST(size_too_small_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
ASSERT_NE(NULL, ctx);
if (attr_lsm_count()) {
@@ -41,7 +41,7 @@ TEST(flags_zero_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
ASSERT_NE(NULL, ctx);
if (attr_lsm_count()) {
@@ -57,7 +57,7 @@ TEST(flags_overset_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
char *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
struct lsm_ctx *tctx = (struct lsm_ctx *)ctx;
ASSERT_NE(NULL, ctx);
^ permalink raw reply related
* Re: [PATCH v2] LSM: use 32 bit compatible data types in LSM syscalls.
From: Casey Schaufler @ 2024-03-13 18:57 UTC (permalink / raw)
To: Paul Moore, Dmitry V. Levin, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <6353ba2abd868cd83186f54e7b71c840@paul-moore.com>
On 3/13/2024 11:46 AM, Paul Moore wrote:
> On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
>> LSM: use 32 bit compatible data types in LSM syscalls.
>>
>> Change the size paramters in lsm_list_modules(), lsm_set_self_attr()
> s/paramters/parameters/
>
>> and lsm_get_self_attr() from size_t to u32. This avoids the need to
>> have different interfaces for 32 and 64 bit systems.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> We should add the following 'Fixes:' tags as well as a stable marking:
>
> Cc: stable@vger.kernel.org
> Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
> Fixes: ad4aff9ec25f ("LSM: Create lsm_list_modules system call")
>
>> ---
>> include/linux/lsm_hook_defs.h | 4 ++--
>> include/linux/security.h | 8 ++++----
>> security/apparmor/lsm.c | 4 ++--
>> security/lsm_syscalls.c | 10 +++++-----
>> security/security.c | 14 +++++++-------
>> security/selinux/hooks.c | 4 ++--
>> security/smack/smack_lsm.c | 4 ++--
>> tools/testing/selftests/lsm/common.h | 6 +++---
>> tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 12 ++++++------
>> tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
>> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
>> 11 files changed, 40 insertions(+), 40 deletions(-)
> ..
>
>> diff --git a/security/security.c b/security/security.c
>> index 7035ee35a393..a0f9caf89ae1 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
>> nctx->ctx_len = val_len;
>> memcpy(nctx->ctx, val, val_len);
>>
>> - if (copy_to_user(uctx, nctx, nctx_len))
>> + if (uctx && copy_to_user(uctx, nctx, nctx_len))
>> rc = -EFAULT;
> Hey, where did that @uctx check come from?
>
> I'm trying to work through if that is a good/bad change, but regardless
> of if we want to make that change, it really should be in a separate
> patch as it has nothing to do with the syscall parameter changes.
>
>> diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
>> index e0e313d9047a..288302a444e0 100644
>> --- a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
>> +++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
>> @@ -76,8 +76,8 @@ TEST(flags_zero_lsm_get_self_attr)
>> {
>> const long page_size = sysconf(_SC_PAGESIZE);
>> struct lsm_ctx *ctx = calloc(page_size, 1);
>> - __u64 *syscall_lsms = calloc(page_size, 1);
>> - size_t size;
>> + __u32 *syscall_lsms = calloc(page_size, 1);
> I believe that should remain a __u64 pointer as we didn't change the
> first parameter to lsm_list_modules(). I'm guessing this was an victim
> of an overzealous /u64/u32/ search-n-replace going from v1 to v2.
>
>> + __u32 size;
>> int lsmcount;
>> int i;
>>
> In the interest of speeding things along, I'm happy to make the above
> changes while merging Casey, but if you would prefer to do a respin
> that's fine with me - let me know either way so I can plan accordingly.
I'll respin. Shouldn't take very long.
^ permalink raw reply
* Re: [PATCH v2] LSM: use 32 bit compatible data types in LSM syscalls.
From: Paul Moore @ 2024-03-13 18:46 UTC (permalink / raw)
To: Casey Schaufler, Dmitry V. Levin, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <045f54ea-4057-43b8-81e2-5cc1b3966d04@schaufler-ca.com>
On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> LSM: use 32 bit compatible data types in LSM syscalls.
>
> Change the size paramters in lsm_list_modules(), lsm_set_self_attr()
s/paramters/parameters/
> and lsm_get_self_attr() from size_t to u32. This avoids the need to
> have different interfaces for 32 and 64 bit systems.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
We should add the following 'Fixes:' tags as well as a stable marking:
Cc: stable@vger.kernel.org
Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
Fixes: ad4aff9ec25f ("LSM: Create lsm_list_modules system call")
> ---
> include/linux/lsm_hook_defs.h | 4 ++--
> include/linux/security.h | 8 ++++----
> security/apparmor/lsm.c | 4 ++--
> security/lsm_syscalls.c | 10 +++++-----
> security/security.c | 14 +++++++-------
> security/selinux/hooks.c | 4 ++--
> security/smack/smack_lsm.c | 4 ++--
> tools/testing/selftests/lsm/common.h | 6 +++---
> tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 12 ++++++------
> tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
> 11 files changed, 40 insertions(+), 40 deletions(-)
...
> diff --git a/security/security.c b/security/security.c
> index 7035ee35a393..a0f9caf89ae1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> nctx->ctx_len = val_len;
> memcpy(nctx->ctx, val, val_len);
>
> - if (copy_to_user(uctx, nctx, nctx_len))
> + if (uctx && copy_to_user(uctx, nctx, nctx_len))
> rc = -EFAULT;
Hey, where did that @uctx check come from?
I'm trying to work through if that is a good/bad change, but regardless
of if we want to make that change, it really should be in a separate
patch as it has nothing to do with the syscall parameter changes.
> diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
> index e0e313d9047a..288302a444e0 100644
> --- a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
> +++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
> @@ -76,8 +76,8 @@ TEST(flags_zero_lsm_get_self_attr)
> {
> const long page_size = sysconf(_SC_PAGESIZE);
> struct lsm_ctx *ctx = calloc(page_size, 1);
> - __u64 *syscall_lsms = calloc(page_size, 1);
> - size_t size;
> + __u32 *syscall_lsms = calloc(page_size, 1);
I believe that should remain a __u64 pointer as we didn't change the
first parameter to lsm_list_modules(). I'm guessing this was an victim
of an overzealous /u64/u32/ search-n-replace going from v1 to v2.
> + __u32 size;
> int lsmcount;
> int i;
>
In the interest of speeding things along, I'm happy to make the above
changes while merging Casey, but if you would prefer to do a respin
that's fine with me - let me know either way so I can plan accordingly.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v3 2/2] VT: Allow to get max font width and height
From: Oleg Bulatov @ 2024-03-13 17:40 UTC (permalink / raw)
To: legion
Cc: Greg Kroah-Hartman, Jiri Slaby, LKML, kbd, linux-api, linux-fbdev,
linux-serial
In-Reply-To: <78fcb9ad77b88edee8768806ce6a4d23f6d33118.1710252966.git.legion@kernel.org>
On Tue, Mar 12, 2024 at 03:23:58PM +0100, legion@kernel.org wrote:
> drivers/video/console/newport_con.c | 21 +++++++++++++++++----
> drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
> drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
> drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
> 4 files changed, 81 insertions(+), 8 deletions(-)
newport_con.c is an interesting one, apparently it's for SGI Indy and
Indigo2, both are discontinued in 1997. Do we still have a way to test
this driver?
Oleg
^ permalink raw reply
* [PATCH v2] LSM: use 32 bit compatible data types in LSM syscalls.
From: Casey Schaufler @ 2024-03-13 15:56 UTC (permalink / raw)
To: Dmitry V. Levin, Paul Moore, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <00734a64-a5fe-420c-bf6e-bee27c9d83be.ref@schaufler-ca.com>
LSM: use 32 bit compatible data types in LSM syscalls.
Change the size paramters in lsm_list_modules(), lsm_set_self_attr()
and lsm_get_self_attr() from size_t to u32. This avoids the need to
have different interfaces for 32 and 64 bit systems.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hook_defs.h | 4 ++--
include/linux/security.h | 8 ++++----
security/apparmor/lsm.c | 4 ++--
security/lsm_syscalls.c | 10 +++++-----
security/security.c | 14 +++++++-------
security/selinux/hooks.c | 4 ++--
security/smack/smack_lsm.c | 4 ++--
tools/testing/selftests/lsm/common.h | 6 +++---
tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 12 ++++++------
tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
11 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..f9b5baf1b5f4 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -265,9 +265,9 @@ LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
struct inode *inode)
LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
- struct lsm_ctx __user *ctx, size_t *size, u32 flags)
+ struct lsm_ctx __user *ctx, u32 *size, u32 flags)
LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
- struct lsm_ctx *ctx, size_t size, u32 flags)
+ struct lsm_ctx *ctx, u32 size, u32 flags)
LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
char **value)
LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..3180d823e023 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -478,9 +478,9 @@ int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
void security_d_instantiate(struct dentry *dentry, struct inode *inode);
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t __user *size, u32 flags);
+ u32 __user *size, u32 flags);
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t size, u32 flags);
+ u32 size, u32 flags);
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value);
int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
@@ -494,7 +494,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
-int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
void *val, size_t val_len, u64 id, u64 flags);
#else /* CONFIG_SECURITY */
@@ -1434,7 +1434,7 @@ static inline int security_locked_down(enum lockdown_reason what)
return 0;
}
static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
- size_t *uctx_len, void *val, size_t val_len,
+ u32 *uctx_len, void *val, size_t val_len,
u64 id, u64 flags)
{
return -EOPNOTSUPP;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 9a3dcaafb5b1..cef8c466af80 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -779,7 +779,7 @@ static int apparmor_sb_pivotroot(const struct path *old_path,
}
static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
- size_t *size, u32 flags)
+ u32 *size, u32 flags)
{
int error = -ENOENT;
struct aa_task_ctx *ctx = task_ctx(current);
@@ -924,7 +924,7 @@ static int do_setattr(u64 attr, void *value, size_t size)
}
static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
int rc;
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 5d391b1f7e69..8440948a690c 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -53,7 +53,7 @@ u64 lsm_name_to_attr(const char *name)
* value indicating the reason for the error is returned.
*/
SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
- ctx, size_t, size, u32, flags)
+ ctx, u32, size, u32, flags)
{
return security_setselfattr(attr, ctx, size, flags);
}
@@ -75,7 +75,7 @@ SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
* a negative value indicating the error is returned.
*/
SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
- ctx, size_t __user *, size, u32, flags)
+ ctx, u32 __user *, size, u32, flags)
{
return security_getselfattr(attr, ctx, size, flags);
}
@@ -93,11 +93,11 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
* required size. In all other cases a negative value indicating the
* error is returned.
*/
-SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
+SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size,
u32, flags)
{
- size_t total_size = lsm_active_cnt * sizeof(*ids);
- size_t usize;
+ u32 total_size = lsm_active_cnt * sizeof(*ids);
+ u32 usize;
int i;
if (flags)
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..a0f9caf89ae1 100644
--- a/security/security.c
+++ b/security/security.c
@@ -785,7 +785,7 @@ static int lsm_superblock_alloc(struct super_block *sb)
* Returns 0 on success, -E2BIG if userspace buffer is not large enough,
* -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
*/
-int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
void *val, size_t val_len,
u64 id, u64 flags)
{
@@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
nctx->ctx_len = val_len;
memcpy(nctx->ctx, val, val_len);
- if (copy_to_user(uctx, nctx, nctx_len))
+ if (uctx && copy_to_user(uctx, nctx, nctx_len))
rc = -EFAULT;
out:
@@ -3918,14 +3918,14 @@ EXPORT_SYMBOL(security_d_instantiate);
* If @size is insufficient to contain the data -E2BIG is returned.
*/
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
- size_t __user *size, u32 flags)
+ u32 __user *size, u32 flags)
{
struct security_hook_list *hp;
struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
u8 __user *base = (u8 __user *)uctx;
- size_t total = 0;
- size_t entrysize;
- size_t left;
+ u32 entrysize;
+ u32 total = 0;
+ u32 left;
bool toobig = false;
bool single = false;
int count = 0;
@@ -4011,7 +4011,7 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
* LSM specific failure.
*/
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
struct security_hook_list *hp;
struct lsm_ctx *lctx;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 338b023a8c3e..71e6e7079d7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6556,7 +6556,7 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
* There will only ever be one attribute.
*/
static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t *size, u32 flags)
+ u32 *size, u32 flags)
{
int rc;
char *val = NULL;
@@ -6571,7 +6571,7 @@ static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
}
static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
int rc;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..0891fcc11e40 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3641,7 +3641,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
* There will only ever be one attribute.
*/
static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t *size, u32 flags)
+ u32 *size, u32 flags)
{
int rc;
struct smack_known *skp;
@@ -3762,7 +3762,7 @@ static int do_setattr(u64 attr, void *value, size_t size)
* Returns 0 on success, an error code otherwise.
*/
static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, u32 flags)
+ u32 size, u32 flags)
{
int rc;
diff --git a/tools/testing/selftests/lsm/common.h b/tools/testing/selftests/lsm/common.h
index d404329e5eeb..06d12110d241 100644
--- a/tools/testing/selftests/lsm/common.h
+++ b/tools/testing/selftests/lsm/common.h
@@ -7,7 +7,7 @@
#ifndef lsm_get_self_attr
static inline int lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
- size_t *size, __u32 flags)
+ __u32 *size, __u32 flags)
{
return syscall(__NR_lsm_get_self_attr, attr, ctx, size, flags);
}
@@ -15,14 +15,14 @@ static inline int lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
#ifndef lsm_set_self_attr
static inline int lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
- size_t size, __u32 flags)
+ __u32 size, __u32 flags)
{
return syscall(__NR_lsm_set_self_attr, attr, ctx, size, flags);
}
#endif
#ifndef lsm_list_modules
-static inline int lsm_list_modules(__u64 *ids, size_t *size, __u32 flags)
+static inline int lsm_list_modules(__u64 *ids, __u32 *size, __u32 flags)
{
return syscall(__NR_lsm_list_modules, ids, size, flags);
}
diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
index e0e313d9047a..288302a444e0 100644
--- a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
+++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
@@ -40,7 +40,7 @@ TEST(size_null_lsm_get_self_attr)
TEST(ctx_null_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
int rc;
rc = lsm_get_self_attr(LSM_ATTR_CURRENT, NULL, &size, 0);
@@ -57,7 +57,7 @@ TEST(size_too_small_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = 1;
+ __u32 size = 1;
ASSERT_NE(NULL, ctx);
errno = 0;
@@ -76,8 +76,8 @@ TEST(flags_zero_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- __u64 *syscall_lsms = calloc(page_size, 1);
- size_t size;
+ __u32 *syscall_lsms = calloc(page_size, 1);
+ __u32 size;
int lsmcount;
int i;
@@ -117,7 +117,7 @@ TEST(flags_overset_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size;
+ __u32 size;
ASSERT_NE(NULL, ctx);
@@ -140,7 +140,7 @@ TEST(flags_overset_lsm_get_self_attr)
TEST(basic_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
struct lsm_ctx *ctx = calloc(page_size, 1);
struct lsm_ctx *tctx = NULL;
__u64 *syscall_lsms = calloc(page_size, 1);
diff --git a/tools/testing/selftests/lsm/lsm_list_modules_test.c b/tools/testing/selftests/lsm/lsm_list_modules_test.c
index 9df29b1e3497..868641dbb309 100644
--- a/tools/testing/selftests/lsm/lsm_list_modules_test.c
+++ b/tools/testing/selftests/lsm/lsm_list_modules_test.c
@@ -31,7 +31,7 @@ TEST(size_null_lsm_list_modules)
TEST(ids_null_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
errno = 0;
ASSERT_EQ(-1, lsm_list_modules(NULL, &size, 0));
@@ -43,7 +43,7 @@ TEST(size_too_small_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size = 1;
+ __u32 size = 1;
ASSERT_NE(NULL, syscall_lsms);
errno = 0;
@@ -58,7 +58,7 @@ TEST(flags_set_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
ASSERT_NE(NULL, syscall_lsms);
errno = 0;
@@ -72,7 +72,7 @@ TEST(flags_set_lsm_list_modules)
TEST(correct_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u32 size = page_size;
__u64 *syscall_lsms = calloc(page_size, 1);
char *sysfs_lsms = calloc(page_size, 1);
char *name;
diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
index e9712c6cf596..66dec47e3ca3 100644
--- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
+++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
@@ -25,7 +25,7 @@ TEST(size_too_small_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
ASSERT_NE(NULL, ctx);
if (attr_lsm_count()) {
@@ -41,7 +41,7 @@ TEST(flags_zero_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
ASSERT_NE(NULL, ctx);
if (attr_lsm_count()) {
@@ -57,7 +57,7 @@ TEST(flags_overset_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
char *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u32 size = page_size;
struct lsm_ctx *tctx = (struct lsm_ctx *)ctx;
ASSERT_NE(NULL, ctx);
^ permalink raw reply related
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Paul Moore @ 2024-03-12 23:17 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dmitry V. Levin, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel,
linux-api, mic
In-Reply-To: <ad5333de-0c89-4191-a217-014f3953a1df@schaufler-ca.com>
On Tue, Mar 12, 2024 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/12/2024 3:06 PM, Paul Moore wrote:
> > On Tue, Mar 12, 2024 at 2:28 PM Dmitry V. Levin <ldv@strace.io> wrote:
> >> On Tue, Mar 12, 2024 at 10:44:38AM -0700, Casey Schaufler wrote:
> >>> On 3/12/2024 10:06 AM, Paul Moore wrote:
> >>>> On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>> On 3/12/2024 6:25 AM, Paul Moore wrote:
> >>>>>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
> >>>>>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
> >>>>>>> [...]
> >>>>>>>> --- a/security/lsm_syscalls.c
> >>>>>>>> +++ b/security/lsm_syscalls.c
> >>>>>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> >>>>>>>> {
> >>>>>>>> return security_getselfattr(attr, ctx, size, flags);
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>> + * sys_lsm_list_modules - Return a list of the active security modules
> >>>>>>>> + * @ids: the LSM module ids
> >>>>>>>> + * @size: pointer to size of @ids, updated on return
> >>>>>>>> + * @flags: reserved for future use, must be zero
> >>>>>>>> + *
> >>>>>>>> + * Returns a list of the active LSM ids. On success this function
> >>>>>>>> + * returns the number of @ids array elements. This value may be zero
> >>>>>>>> + * if there are no LSMs active. If @size is insufficient to contain
> >>>>>>>> + * the return data -E2BIG is returned and @size is set to the minimum
> >>>>>>>> + * required size. In all other cases a negative value indicating the
> >>>>>>>> + * error is returned.
> >>>>>>>> + */
> >>>>>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
> >>>>>>>> + u32, flags)
> >>>>>>> I'm sorry but the size of userspace size_t is different from the kernel one
> >>>>>>> on 32-bit compat architectures.
> >>>>>> D'oh, yes, thanks for pointing that out. It would have been nice to
> >>>>>> have caught that before v6.8 was released, but I guess it's better
> >>>>>> than later.
> >>>>>>
> >>>>>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
> >>>>>>> now. Other two added lsm syscalls also have this issue.
> >>>>>> Considering that Linux v6.8, and by extension these syscalls, are only
> >>>>>> a few days old, I think I'd rather see us just modify the syscalls and
> >>>>>> avoid the compat baggage. I'm going to be shocked if anyone has
> >>>>>> shifted to using the new syscalls yet, and even if they have (!!),
> >>>>>> moving from a "size_t" type to a "u64" should be mostly transparent
> >>>>>> for the majority of native 64-bit systems. Those running the absolute
> >>>>>> latest kernels on 32-bit systems with custom or bleeding edge
> >>>>>> userspace *may* see a slight hiccup, but I think that user count is in
> >>>>>> the single digits, if not zero.
> >>>>>>
> >>>>>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> >>>>>> compat shim if we can.
> >>>>>>
> >>>>>> Casey, do you have time to put together a patch for this (you should
> >>>>>> fix the call chains below the syscalls too)? If not, please let me
> >>>>>> know and I'll get a patch out ASAP.
> >>>>> Grumble. Yes, I'll get right on it.
> >>>> Great, thanks Casey.
> >>> Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
> >>> doesn't, need it, but I'm tempted to change it as well for consistency.
> >>> Thoughts?
> >> As lsm_get_self_attr() has the same issue, it needs the same treatment.
> >>
> >> lsm_set_self_attr() could be left unchanged. In fact, changing the type
> >> of syscall arguments from size_t to an explicit 64-bit type would be
> >> problematic because 32-bit syscalls cannot have 64-bit arguments.
> > You might as well convert all of the size_t parameters, pointers or
> > otherwise, in the three syscalls to u32 Casey.
>
> Well, nuts. So much for that coin flip. V2 coming real soon.
Yeah, sorry for the false starts today ...
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Casey Schaufler @ 2024-03-12 22:17 UTC (permalink / raw)
To: Paul Moore, Dmitry V. Levin
Cc: linux-security-module, jmorris, serge, keescook, john.johansen,
penguin-kernel, stephen.smalley.work, linux-kernel, linux-api,
mic, Casey Schaufler
In-Reply-To: <CAHC9VhTw2h6V3Z4fut8vHf59kOJeYqDrPqbRmRDNtZmVnM4g6Q@mail.gmail.com>
On 3/12/2024 3:06 PM, Paul Moore wrote:
> On Tue, Mar 12, 2024 at 2:28 PM Dmitry V. Levin <ldv@strace.io> wrote:
>> On Tue, Mar 12, 2024 at 10:44:38AM -0700, Casey Schaufler wrote:
>>> On 3/12/2024 10:06 AM, Paul Moore wrote:
>>>> On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 3/12/2024 6:25 AM, Paul Moore wrote:
>>>>>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
>>>>>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
>>>>>>> [...]
>>>>>>>> --- a/security/lsm_syscalls.c
>>>>>>>> +++ b/security/lsm_syscalls.c
>>>>>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
>>>>>>>> {
>>>>>>>> return security_getselfattr(attr, ctx, size, flags);
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * sys_lsm_list_modules - Return a list of the active security modules
>>>>>>>> + * @ids: the LSM module ids
>>>>>>>> + * @size: pointer to size of @ids, updated on return
>>>>>>>> + * @flags: reserved for future use, must be zero
>>>>>>>> + *
>>>>>>>> + * Returns a list of the active LSM ids. On success this function
>>>>>>>> + * returns the number of @ids array elements. This value may be zero
>>>>>>>> + * if there are no LSMs active. If @size is insufficient to contain
>>>>>>>> + * the return data -E2BIG is returned and @size is set to the minimum
>>>>>>>> + * required size. In all other cases a negative value indicating the
>>>>>>>> + * error is returned.
>>>>>>>> + */
>>>>>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
>>>>>>>> + u32, flags)
>>>>>>> I'm sorry but the size of userspace size_t is different from the kernel one
>>>>>>> on 32-bit compat architectures.
>>>>>> D'oh, yes, thanks for pointing that out. It would have been nice to
>>>>>> have caught that before v6.8 was released, but I guess it's better
>>>>>> than later.
>>>>>>
>>>>>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
>>>>>>> now. Other two added lsm syscalls also have this issue.
>>>>>> Considering that Linux v6.8, and by extension these syscalls, are only
>>>>>> a few days old, I think I'd rather see us just modify the syscalls and
>>>>>> avoid the compat baggage. I'm going to be shocked if anyone has
>>>>>> shifted to using the new syscalls yet, and even if they have (!!),
>>>>>> moving from a "size_t" type to a "u64" should be mostly transparent
>>>>>> for the majority of native 64-bit systems. Those running the absolute
>>>>>> latest kernels on 32-bit systems with custom or bleeding edge
>>>>>> userspace *may* see a slight hiccup, but I think that user count is in
>>>>>> the single digits, if not zero.
>>>>>>
>>>>>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
>>>>>> compat shim if we can.
>>>>>>
>>>>>> Casey, do you have time to put together a patch for this (you should
>>>>>> fix the call chains below the syscalls too)? If not, please let me
>>>>>> know and I'll get a patch out ASAP.
>>>>> Grumble. Yes, I'll get right on it.
>>>> Great, thanks Casey.
>>> Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
>>> doesn't, need it, but I'm tempted to change it as well for consistency.
>>> Thoughts?
>> As lsm_get_self_attr() has the same issue, it needs the same treatment.
>>
>> lsm_set_self_attr() could be left unchanged. In fact, changing the type
>> of syscall arguments from size_t to an explicit 64-bit type would be
>> problematic because 32-bit syscalls cannot have 64-bit arguments.
> You might as well convert all of the size_t parameters, pointers or
> otherwise, in the three syscalls to u32 Casey.
Well, nuts. So much for that coin flip. V2 coming real soon.
^ permalink raw reply
* [PATCH] LSM: use 32 bit compatible data types in LSM syscalls.
From: Casey Schaufler @ 2024-03-12 22:13 UTC (permalink / raw)
To: Dmitry V. Levin, Paul Moore, LSM List
Cc: Linux kernel mailing list, linux-api, Mickaël Salaün,
James Morris, Serge Hallyn, John Johansen, Tetsuo Handa,
Stephen Smalley, Casey Schaufler
In-Reply-To: <00734a64-a5fe-420c-bf6e-bee27c9d83be.ref@schaufler-ca.com>
Change the size paramters in lsm_list_modules() and lsm_get_self_attr()
from size_t to u64. This avoids the need to have different interfaces
for 32 and 64 bit systems.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hook_defs.h | 2 +-
include/linux/security.h | 6 +++---
security/apparmor/lsm.c | 2 +-
security/lsm_syscalls.c | 8 ++++----
security/security.c | 6 +++---
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
tools/testing/selftests/lsm/common.h | 4 ++--
tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
11 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..1d3b498f5230 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -265,7 +265,7 @@ LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
struct inode *inode)
LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
- struct lsm_ctx __user *ctx, size_t *size, u32 flags)
+ struct lsm_ctx __user *ctx, u64 *size, u32 flags)
LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
struct lsm_ctx *ctx, size_t size, u32 flags)
LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..d5044c290b4c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -478,7 +478,7 @@ int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
void security_d_instantiate(struct dentry *dentry, struct inode *inode);
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t __user *size, u32 flags);
+ u64 __user *size, u32 flags);
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
size_t size, u32 flags);
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
@@ -494,7 +494,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
-int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u64 *uctx_len,
void *val, size_t val_len, u64 id, u64 flags);
#else /* CONFIG_SECURITY */
@@ -1434,7 +1434,7 @@ static inline int security_locked_down(enum lockdown_reason what)
return 0;
}
static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
- size_t *uctx_len, void *val, size_t val_len,
+ u64 *uctx_len, void *val, size_t val_len,
u64 id, u64 flags)
{
return -EOPNOTSUPP;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 9a3dcaafb5b1..77d1293f6a44 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -779,7 +779,7 @@ static int apparmor_sb_pivotroot(const struct path *old_path,
}
static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
- size_t *size, u32 flags)
+ u64 *size, u32 flags)
{
int error = -ENOENT;
struct aa_task_ctx *ctx = task_ctx(current);
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 5d391b1f7e69..3a76f335e46e 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -75,7 +75,7 @@ SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
* a negative value indicating the error is returned.
*/
SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
- ctx, size_t __user *, size, u32, flags)
+ ctx, u64 __user *, size, u32, flags)
{
return security_getselfattr(attr, ctx, size, flags);
}
@@ -93,11 +93,11 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
* required size. In all other cases a negative value indicating the
* error is returned.
*/
-SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
+SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u64 __user *, size,
u32, flags)
{
- size_t total_size = lsm_active_cnt * sizeof(*ids);
- size_t usize;
+ u64 total_size = lsm_active_cnt * sizeof(*ids);
+ u64 usize;
int i;
if (flags)
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..7721330f6d65 100644
--- a/security/security.c
+++ b/security/security.c
@@ -785,7 +785,7 @@ static int lsm_superblock_alloc(struct super_block *sb)
* Returns 0 on success, -E2BIG if userspace buffer is not large enough,
* -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
*/
-int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u64 *uctx_len,
void *val, size_t val_len,
u64 id, u64 flags)
{
@@ -3918,13 +3918,13 @@ EXPORT_SYMBOL(security_d_instantiate);
* If @size is insufficient to contain the data -E2BIG is returned.
*/
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
- size_t __user *size, u32 flags)
+ u64 __user *size, u32 flags)
{
struct security_hook_list *hp;
struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
u8 __user *base = (u8 __user *)uctx;
+ u64 entrysize;
size_t total = 0;
- size_t entrysize;
size_t left;
bool toobig = false;
bool single = false;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 338b023a8c3e..92677eb3ed31 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6556,7 +6556,7 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
* There will only ever be one attribute.
*/
static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t *size, u32 flags)
+ u64 *size, u32 flags)
{
int rc;
char *val = NULL;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..0bb295184806 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3641,7 +3641,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
* There will only ever be one attribute.
*/
static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
- size_t *size, u32 flags)
+ u64 *size, u32 flags)
{
int rc;
struct smack_known *skp;
diff --git a/tools/testing/selftests/lsm/common.h b/tools/testing/selftests/lsm/common.h
index d404329e5eeb..af9fd720a9fb 100644
--- a/tools/testing/selftests/lsm/common.h
+++ b/tools/testing/selftests/lsm/common.h
@@ -7,7 +7,7 @@
#ifndef lsm_get_self_attr
static inline int lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
- size_t *size, __u32 flags)
+ __u64 *size, __u32 flags)
{
return syscall(__NR_lsm_get_self_attr, attr, ctx, size, flags);
}
@@ -22,7 +22,7 @@ static inline int lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
#endif
#ifndef lsm_list_modules
-static inline int lsm_list_modules(__u64 *ids, size_t *size, __u32 flags)
+static inline int lsm_list_modules(__u64 *ids, __u64 *size, __u32 flags)
{
return syscall(__NR_lsm_list_modules, ids, size, flags);
}
diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
index e0e313d9047a..20be5454ad05 100644
--- a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
+++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
@@ -40,7 +40,7 @@ TEST(size_null_lsm_get_self_attr)
TEST(ctx_null_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u64 size = page_size;
int rc;
rc = lsm_get_self_attr(LSM_ATTR_CURRENT, NULL, &size, 0);
@@ -57,7 +57,7 @@ TEST(size_too_small_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = 1;
+ __u64 size = 1;
ASSERT_NE(NULL, ctx);
errno = 0;
@@ -77,7 +77,7 @@ TEST(flags_zero_lsm_get_self_attr)
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size;
+ __u64 size;
int lsmcount;
int i;
@@ -117,7 +117,7 @@ TEST(flags_overset_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size;
+ __u64 size;
ASSERT_NE(NULL, ctx);
@@ -140,7 +140,7 @@ TEST(flags_overset_lsm_get_self_attr)
TEST(basic_lsm_get_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u64 size = page_size;
struct lsm_ctx *ctx = calloc(page_size, 1);
struct lsm_ctx *tctx = NULL;
__u64 *syscall_lsms = calloc(page_size, 1);
diff --git a/tools/testing/selftests/lsm/lsm_list_modules_test.c b/tools/testing/selftests/lsm/lsm_list_modules_test.c
index 9df29b1e3497..52d014b0ff37 100644
--- a/tools/testing/selftests/lsm/lsm_list_modules_test.c
+++ b/tools/testing/selftests/lsm/lsm_list_modules_test.c
@@ -31,7 +31,7 @@ TEST(size_null_lsm_list_modules)
TEST(ids_null_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u64 size = page_size;
errno = 0;
ASSERT_EQ(-1, lsm_list_modules(NULL, &size, 0));
@@ -43,7 +43,7 @@ TEST(size_too_small_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size = 1;
+ __u64 size = 1;
ASSERT_NE(NULL, syscall_lsms);
errno = 0;
@@ -58,7 +58,7 @@ TEST(flags_set_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
__u64 *syscall_lsms = calloc(page_size, 1);
- size_t size = page_size;
+ __u64 size = page_size;
ASSERT_NE(NULL, syscall_lsms);
errno = 0;
@@ -72,7 +72,7 @@ TEST(flags_set_lsm_list_modules)
TEST(correct_lsm_list_modules)
{
const long page_size = sysconf(_SC_PAGESIZE);
- size_t size = page_size;
+ __u64 size = page_size;
__u64 *syscall_lsms = calloc(page_size, 1);
char *sysfs_lsms = calloc(page_size, 1);
char *name;
diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
index e9712c6cf596..ec7a4df6d4d9 100644
--- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
+++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
@@ -25,7 +25,7 @@ TEST(size_too_small_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u64 size = page_size;
ASSERT_NE(NULL, ctx);
if (attr_lsm_count()) {
@@ -41,7 +41,7 @@ TEST(flags_zero_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
struct lsm_ctx *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u64 size = page_size;
ASSERT_NE(NULL, ctx);
if (attr_lsm_count()) {
@@ -57,7 +57,7 @@ TEST(flags_overset_lsm_set_self_attr)
{
const long page_size = sysconf(_SC_PAGESIZE);
char *ctx = calloc(page_size, 1);
- size_t size = page_size;
+ __u64 size = page_size;
struct lsm_ctx *tctx = (struct lsm_ctx *)ctx;
ASSERT_NE(NULL, ctx);
^ permalink raw reply related
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Paul Moore @ 2024-03-12 22:06 UTC (permalink / raw)
To: Casey Schaufler, Dmitry V. Levin
Cc: linux-security-module, jmorris, serge, keescook, john.johansen,
penguin-kernel, stephen.smalley.work, linux-kernel, linux-api,
mic
In-Reply-To: <20240312182820.GA5122@altlinux.org>
On Tue, Mar 12, 2024 at 2:28 PM Dmitry V. Levin <ldv@strace.io> wrote:
> On Tue, Mar 12, 2024 at 10:44:38AM -0700, Casey Schaufler wrote:
> > On 3/12/2024 10:06 AM, Paul Moore wrote:
> > > On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> On 3/12/2024 6:25 AM, Paul Moore wrote:
> > >>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
> > >>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
> > >>>> [...]
> > >>>>> --- a/security/lsm_syscalls.c
> > >>>>> +++ b/security/lsm_syscalls.c
> > >>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> > >>>>> {
> > >>>>> return security_getselfattr(attr, ctx, size, flags);
> > >>>>> }
> > >>>>> +
> > >>>>> +/**
> > >>>>> + * sys_lsm_list_modules - Return a list of the active security modules
> > >>>>> + * @ids: the LSM module ids
> > >>>>> + * @size: pointer to size of @ids, updated on return
> > >>>>> + * @flags: reserved for future use, must be zero
> > >>>>> + *
> > >>>>> + * Returns a list of the active LSM ids. On success this function
> > >>>>> + * returns the number of @ids array elements. This value may be zero
> > >>>>> + * if there are no LSMs active. If @size is insufficient to contain
> > >>>>> + * the return data -E2BIG is returned and @size is set to the minimum
> > >>>>> + * required size. In all other cases a negative value indicating the
> > >>>>> + * error is returned.
> > >>>>> + */
> > >>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
> > >>>>> + u32, flags)
> > >>>> I'm sorry but the size of userspace size_t is different from the kernel one
> > >>>> on 32-bit compat architectures.
> > >>> D'oh, yes, thanks for pointing that out. It would have been nice to
> > >>> have caught that before v6.8 was released, but I guess it's better
> > >>> than later.
> > >>>
> > >>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
> > >>>> now. Other two added lsm syscalls also have this issue.
> > >>> Considering that Linux v6.8, and by extension these syscalls, are only
> > >>> a few days old, I think I'd rather see us just modify the syscalls and
> > >>> avoid the compat baggage. I'm going to be shocked if anyone has
> > >>> shifted to using the new syscalls yet, and even if they have (!!),
> > >>> moving from a "size_t" type to a "u64" should be mostly transparent
> > >>> for the majority of native 64-bit systems. Those running the absolute
> > >>> latest kernels on 32-bit systems with custom or bleeding edge
> > >>> userspace *may* see a slight hiccup, but I think that user count is in
> > >>> the single digits, if not zero.
> > >>>
> > >>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> > >>> compat shim if we can.
> > >>>
> > >>> Casey, do you have time to put together a patch for this (you should
> > >>> fix the call chains below the syscalls too)? If not, please let me
> > >>> know and I'll get a patch out ASAP.
> > >> Grumble. Yes, I'll get right on it.
> > > Great, thanks Casey.
> >
> > Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
> > doesn't, need it, but I'm tempted to change it as well for consistency.
> > Thoughts?
>
> As lsm_get_self_attr() has the same issue, it needs the same treatment.
>
> lsm_set_self_attr() could be left unchanged. In fact, changing the type
> of syscall arguments from size_t to an explicit 64-bit type would be
> problematic because 32-bit syscalls cannot have 64-bit arguments.
You might as well convert all of the size_t parameters, pointers or
otherwise, in the three syscalls to u32 Casey.
I'd leave the lsm_ctx struct alone, the individual fields are nicely
aligned on both 32-bit and 64-bit systems and worst case we have some
unused bits.
The 64-bit LSM IDs are perhaps a bit more problematic, but I believe
we are okay and I don't think we should change that. With one of the
primary motivations behind the LSM syscalls being support for multiple
LSMs, I suspect any future LSMs will use an array of LSM IDs (the u64
is hidden behind a pointer) as we do in lsm_list_modules() or the LSM
ID will be part of a larger struct like lsm_ctx.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Casey Schaufler @ 2024-03-12 22:06 UTC (permalink / raw)
To: Kees Cook, Dmitry V. Levin
Cc: Paul Moore, linux-security-module, jmorris, serge, john.johansen,
penguin-kernel, stephen.smalley.work, linux-kernel, linux-api,
mic, Casey Schaufler
In-Reply-To: <202403121449.17AB66665@keescook>
On 3/12/2024 2:50 PM, Kees Cook wrote:
> On Tue, Mar 12, 2024 at 08:28:20PM +0200, Dmitry V. Levin wrote:
>> On Tue, Mar 12, 2024 at 10:44:38AM -0700, Casey Schaufler wrote:
>>> On 3/12/2024 10:06 AM, Paul Moore wrote:
>>>> On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 3/12/2024 6:25 AM, Paul Moore wrote:
>>>>>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
>>>>>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
>>>>>>> [...]
>>>>>>>> --- a/security/lsm_syscalls.c
>>>>>>>> +++ b/security/lsm_syscalls.c
>>>>>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
>>>>>>>> {
>>>>>>>> return security_getselfattr(attr, ctx, size, flags);
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * sys_lsm_list_modules - Return a list of the active security modules
>>>>>>>> + * @ids: the LSM module ids
>>>>>>>> + * @size: pointer to size of @ids, updated on return
>>>>>>>> + * @flags: reserved for future use, must be zero
>>>>>>>> + *
>>>>>>>> + * Returns a list of the active LSM ids. On success this function
>>>>>>>> + * returns the number of @ids array elements. This value may be zero
>>>>>>>> + * if there are no LSMs active. If @size is insufficient to contain
>>>>>>>> + * the return data -E2BIG is returned and @size is set to the minimum
>>>>>>>> + * required size. In all other cases a negative value indicating the
>>>>>>>> + * error is returned.
>>>>>>>> + */
>>>>>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
>>>>>>>> + u32, flags)
>>>>>>> I'm sorry but the size of userspace size_t is different from the kernel one
>>>>>>> on 32-bit compat architectures.
>>>>>> D'oh, yes, thanks for pointing that out. It would have been nice to
>>>>>> have caught that before v6.8 was released, but I guess it's better
>>>>>> than later.
>>>>>>
>>>>>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
>>>>>>> now. Other two added lsm syscalls also have this issue.
>>>>>> Considering that Linux v6.8, and by extension these syscalls, are only
>>>>>> a few days old, I think I'd rather see us just modify the syscalls and
>>>>>> avoid the compat baggage. I'm going to be shocked if anyone has
>>>>>> shifted to using the new syscalls yet, and even if they have (!!),
>>>>>> moving from a "size_t" type to a "u64" should be mostly transparent
>>>>>> for the majority of native 64-bit systems. Those running the absolute
>>>>>> latest kernels on 32-bit systems with custom or bleeding edge
>>>>>> userspace *may* see a slight hiccup, but I think that user count is in
>>>>>> the single digits, if not zero.
>>>>>>
>>>>>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
>>>>>> compat shim if we can.
>>>>>>
>>>>>> Casey, do you have time to put together a patch for this (you should
>>>>>> fix the call chains below the syscalls too)? If not, please let me
>>>>>> know and I'll get a patch out ASAP.
>>>>> Grumble. Yes, I'll get right on it.
>>>> Great, thanks Casey.
>>> Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
>>> doesn't, need it, but I'm tempted to change it as well for consistency.
>>> Thoughts?
>> As lsm_get_self_attr() has the same issue, it needs the same treatment.
>>
>> lsm_set_self_attr() could be left unchanged. In fact, changing the type
>> of syscall arguments from size_t to an explicit 64-bit type would be
>> problematic because 32-bit syscalls cannot have 64-bit arguments.
> Using u32 should be totally fine for both. Nearly ever kernel internal
> limits sizes to INT_MAX anyway. :)
My initial patch changes the size_t pointers in lsm_list_modules()
and lsm_get_self_attr() to __u64 pointers. I could change them to
__u32 pointers and include lsm_set_self_attr() as well. Let's see
who screams how loudly.
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Kees Cook @ 2024-03-12 21:50 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Casey Schaufler, Paul Moore, linux-security-module, jmorris,
serge, john.johansen, penguin-kernel, stephen.smalley.work,
linux-kernel, linux-api, mic
In-Reply-To: <20240312182820.GA5122@altlinux.org>
On Tue, Mar 12, 2024 at 08:28:20PM +0200, Dmitry V. Levin wrote:
> On Tue, Mar 12, 2024 at 10:44:38AM -0700, Casey Schaufler wrote:
> > On 3/12/2024 10:06 AM, Paul Moore wrote:
> > > On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> On 3/12/2024 6:25 AM, Paul Moore wrote:
> > >>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
> > >>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
> > >>>> [...]
> > >>>>> --- a/security/lsm_syscalls.c
> > >>>>> +++ b/security/lsm_syscalls.c
> > >>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> > >>>>> {
> > >>>>> return security_getselfattr(attr, ctx, size, flags);
> > >>>>> }
> > >>>>> +
> > >>>>> +/**
> > >>>>> + * sys_lsm_list_modules - Return a list of the active security modules
> > >>>>> + * @ids: the LSM module ids
> > >>>>> + * @size: pointer to size of @ids, updated on return
> > >>>>> + * @flags: reserved for future use, must be zero
> > >>>>> + *
> > >>>>> + * Returns a list of the active LSM ids. On success this function
> > >>>>> + * returns the number of @ids array elements. This value may be zero
> > >>>>> + * if there are no LSMs active. If @size is insufficient to contain
> > >>>>> + * the return data -E2BIG is returned and @size is set to the minimum
> > >>>>> + * required size. In all other cases a negative value indicating the
> > >>>>> + * error is returned.
> > >>>>> + */
> > >>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
> > >>>>> + u32, flags)
> > >>>> I'm sorry but the size of userspace size_t is different from the kernel one
> > >>>> on 32-bit compat architectures.
> > >>> D'oh, yes, thanks for pointing that out. It would have been nice to
> > >>> have caught that before v6.8 was released, but I guess it's better
> > >>> than later.
> > >>>
> > >>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
> > >>>> now. Other two added lsm syscalls also have this issue.
> > >>> Considering that Linux v6.8, and by extension these syscalls, are only
> > >>> a few days old, I think I'd rather see us just modify the syscalls and
> > >>> avoid the compat baggage. I'm going to be shocked if anyone has
> > >>> shifted to using the new syscalls yet, and even if they have (!!),
> > >>> moving from a "size_t" type to a "u64" should be mostly transparent
> > >>> for the majority of native 64-bit systems. Those running the absolute
> > >>> latest kernels on 32-bit systems with custom or bleeding edge
> > >>> userspace *may* see a slight hiccup, but I think that user count is in
> > >>> the single digits, if not zero.
> > >>>
> > >>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> > >>> compat shim if we can.
> > >>>
> > >>> Casey, do you have time to put together a patch for this (you should
> > >>> fix the call chains below the syscalls too)? If not, please let me
> > >>> know and I'll get a patch out ASAP.
> > >> Grumble. Yes, I'll get right on it.
> > > Great, thanks Casey.
> >
> > Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
> > doesn't, need it, but I'm tempted to change it as well for consistency.
> > Thoughts?
>
> As lsm_get_self_attr() has the same issue, it needs the same treatment.
>
> lsm_set_self_attr() could be left unchanged. In fact, changing the type
> of syscall arguments from size_t to an explicit 64-bit type would be
> problematic because 32-bit syscalls cannot have 64-bit arguments.
Using u32 should be totally fine for both. Nearly ever kernel internal
limits sizes to INT_MAX anyway. :)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Dmitry V. Levin @ 2024-03-12 18:28 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel,
linux-api, mic
In-Reply-To: <f4f5d993-552b-483a-9a3e-1be99ea48757@schaufler-ca.com>
On Tue, Mar 12, 2024 at 10:44:38AM -0700, Casey Schaufler wrote:
> On 3/12/2024 10:06 AM, Paul Moore wrote:
> > On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 3/12/2024 6:25 AM, Paul Moore wrote:
> >>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
> >>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
> >>>> [...]
> >>>>> --- a/security/lsm_syscalls.c
> >>>>> +++ b/security/lsm_syscalls.c
> >>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> >>>>> {
> >>>>> return security_getselfattr(attr, ctx, size, flags);
> >>>>> }
> >>>>> +
> >>>>> +/**
> >>>>> + * sys_lsm_list_modules - Return a list of the active security modules
> >>>>> + * @ids: the LSM module ids
> >>>>> + * @size: pointer to size of @ids, updated on return
> >>>>> + * @flags: reserved for future use, must be zero
> >>>>> + *
> >>>>> + * Returns a list of the active LSM ids. On success this function
> >>>>> + * returns the number of @ids array elements. This value may be zero
> >>>>> + * if there are no LSMs active. If @size is insufficient to contain
> >>>>> + * the return data -E2BIG is returned and @size is set to the minimum
> >>>>> + * required size. In all other cases a negative value indicating the
> >>>>> + * error is returned.
> >>>>> + */
> >>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
> >>>>> + u32, flags)
> >>>> I'm sorry but the size of userspace size_t is different from the kernel one
> >>>> on 32-bit compat architectures.
> >>> D'oh, yes, thanks for pointing that out. It would have been nice to
> >>> have caught that before v6.8 was released, but I guess it's better
> >>> than later.
> >>>
> >>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
> >>>> now. Other two added lsm syscalls also have this issue.
> >>> Considering that Linux v6.8, and by extension these syscalls, are only
> >>> a few days old, I think I'd rather see us just modify the syscalls and
> >>> avoid the compat baggage. I'm going to be shocked if anyone has
> >>> shifted to using the new syscalls yet, and even if they have (!!),
> >>> moving from a "size_t" type to a "u64" should be mostly transparent
> >>> for the majority of native 64-bit systems. Those running the absolute
> >>> latest kernels on 32-bit systems with custom or bleeding edge
> >>> userspace *may* see a slight hiccup, but I think that user count is in
> >>> the single digits, if not zero.
> >>>
> >>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> >>> compat shim if we can.
> >>>
> >>> Casey, do you have time to put together a patch for this (you should
> >>> fix the call chains below the syscalls too)? If not, please let me
> >>> know and I'll get a patch out ASAP.
> >> Grumble. Yes, I'll get right on it.
> > Great, thanks Casey.
>
> Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
> doesn't, need it, but I'm tempted to change it as well for consistency.
> Thoughts?
As lsm_get_self_attr() has the same issue, it needs the same treatment.
lsm_set_self_attr() could be left unchanged. In fact, changing the type
of syscall arguments from size_t to an explicit 64-bit type would be
problematic because 32-bit syscalls cannot have 64-bit arguments.
--
ldv
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Paul Moore @ 2024-03-12 18:09 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dmitry V. Levin, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel,
linux-api, mic
In-Reply-To: <f4f5d993-552b-483a-9a3e-1be99ea48757@schaufler-ca.com>
On Tue, Mar 12, 2024 at 1:44 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/12/2024 10:06 AM, Paul Moore wrote:
> > On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 3/12/2024 6:25 AM, Paul Moore wrote:
> >>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
> >>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
> >>>> [...]
> >>>>> --- a/security/lsm_syscalls.c
> >>>>> +++ b/security/lsm_syscalls.c
> >>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> >>>>> {
> >>>>> return security_getselfattr(attr, ctx, size, flags);
> >>>>> }
> >>>>> +
> >>>>> +/**
> >>>>> + * sys_lsm_list_modules - Return a list of the active security modules
> >>>>> + * @ids: the LSM module ids
> >>>>> + * @size: pointer to size of @ids, updated on return
> >>>>> + * @flags: reserved for future use, must be zero
> >>>>> + *
> >>>>> + * Returns a list of the active LSM ids. On success this function
> >>>>> + * returns the number of @ids array elements. This value may be zero
> >>>>> + * if there are no LSMs active. If @size is insufficient to contain
> >>>>> + * the return data -E2BIG is returned and @size is set to the minimum
> >>>>> + * required size. In all other cases a negative value indicating the
> >>>>> + * error is returned.
> >>>>> + */
> >>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
> >>>>> + u32, flags)
> >>>> I'm sorry but the size of userspace size_t is different from the kernel one
> >>>> on 32-bit compat architectures.
> >>> D'oh, yes, thanks for pointing that out. It would have been nice to
> >>> have caught that before v6.8 was released, but I guess it's better
> >>> than later.
> >>>
> >>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
> >>>> now. Other two added lsm syscalls also have this issue.
> >>> Considering that Linux v6.8, and by extension these syscalls, are only
> >>> a few days old, I think I'd rather see us just modify the syscalls and
> >>> avoid the compat baggage. I'm going to be shocked if anyone has
> >>> shifted to using the new syscalls yet, and even if they have (!!),
> >>> moving from a "size_t" type to a "u64" should be mostly transparent
> >>> for the majority of native 64-bit systems. Those running the absolute
> >>> latest kernels on 32-bit systems with custom or bleeding edge
> >>> userspace *may* see a slight hiccup, but I think that user count is in
> >>> the single digits, if not zero.
> >>>
> >>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> >>> compat shim if we can.
> >>>
> >>> Casey, do you have time to put together a patch for this (you should
> >>> fix the call chains below the syscalls too)? If not, please let me
> >>> know and I'll get a patch out ASAP.
> >> Grumble. Yes, I'll get right on it.
> > Great, thanks Casey.
>
> Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
> doesn't, need it, but I'm tempted to change it as well for consistency.
> Thoughts?
I'd suggest changing both.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Casey Schaufler @ 2024-03-12 17:44 UTC (permalink / raw)
To: Paul Moore
Cc: Dmitry V. Levin, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel,
linux-api, mic, Casey Schaufler
In-Reply-To: <CAHC9VhRfwjsGiHXBRcWA6S9+H_kj0vMdQC0gyHr3ZnX-u7KzRQ@mail.gmail.com>
On 3/12/2024 10:06 AM, Paul Moore wrote:
> On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 3/12/2024 6:25 AM, Paul Moore wrote:
>>> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
>>>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
>>>> [...]
>>>>> --- a/security/lsm_syscalls.c
>>>>> +++ b/security/lsm_syscalls.c
>>>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
>>>>> {
>>>>> return security_getselfattr(attr, ctx, size, flags);
>>>>> }
>>>>> +
>>>>> +/**
>>>>> + * sys_lsm_list_modules - Return a list of the active security modules
>>>>> + * @ids: the LSM module ids
>>>>> + * @size: pointer to size of @ids, updated on return
>>>>> + * @flags: reserved for future use, must be zero
>>>>> + *
>>>>> + * Returns a list of the active LSM ids. On success this function
>>>>> + * returns the number of @ids array elements. This value may be zero
>>>>> + * if there are no LSMs active. If @size is insufficient to contain
>>>>> + * the return data -E2BIG is returned and @size is set to the minimum
>>>>> + * required size. In all other cases a negative value indicating the
>>>>> + * error is returned.
>>>>> + */
>>>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
>>>>> + u32, flags)
>>>> I'm sorry but the size of userspace size_t is different from the kernel one
>>>> on 32-bit compat architectures.
>>> D'oh, yes, thanks for pointing that out. It would have been nice to
>>> have caught that before v6.8 was released, but I guess it's better
>>> than later.
>>>
>>>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ..)
>>>> now. Other two added lsm syscalls also have this issue.
>>> Considering that Linux v6.8, and by extension these syscalls, are only
>>> a few days old, I think I'd rather see us just modify the syscalls and
>>> avoid the compat baggage. I'm going to be shocked if anyone has
>>> shifted to using the new syscalls yet, and even if they have (!!),
>>> moving from a "size_t" type to a "u64" should be mostly transparent
>>> for the majority of native 64-bit systems. Those running the absolute
>>> latest kernels on 32-bit systems with custom or bleeding edge
>>> userspace *may* see a slight hiccup, but I think that user count is in
>>> the single digits, if not zero.
>>>
>>> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
>>> compat shim if we can.
>>>
>>> Casey, do you have time to put together a patch for this (you should
>>> fix the call chains below the syscalls too)? If not, please let me
>>> know and I'll get a patch out ASAP.
>> Grumble. Yes, I'll get right on it.
> Great, thanks Casey.
Look like lsm_get_self_attr() needs the same change. lsm_set_self_attr()
doesn't, need it, but I'm tempted to change it as well for consistency.
Thoughts?
>
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Paul Moore @ 2024-03-12 17:06 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dmitry V. Levin, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel,
linux-api, mic
In-Reply-To: <f122b3a9-1208-4c0b-9289-73eb070a8337@schaufler-ca.com>
On Tue, Mar 12, 2024 at 11:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/12/2024 6:25 AM, Paul Moore wrote:
> > On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
> >> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
> >> [...]
> >>> --- a/security/lsm_syscalls.c
> >>> +++ b/security/lsm_syscalls.c
> >>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> >>> {
> >>> return security_getselfattr(attr, ctx, size, flags);
> >>> }
> >>> +
> >>> +/**
> >>> + * sys_lsm_list_modules - Return a list of the active security modules
> >>> + * @ids: the LSM module ids
> >>> + * @size: pointer to size of @ids, updated on return
> >>> + * @flags: reserved for future use, must be zero
> >>> + *
> >>> + * Returns a list of the active LSM ids. On success this function
> >>> + * returns the number of @ids array elements. This value may be zero
> >>> + * if there are no LSMs active. If @size is insufficient to contain
> >>> + * the return data -E2BIG is returned and @size is set to the minimum
> >>> + * required size. In all other cases a negative value indicating the
> >>> + * error is returned.
> >>> + */
> >>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
> >>> + u32, flags)
> >> I'm sorry but the size of userspace size_t is different from the kernel one
> >> on 32-bit compat architectures.
> > D'oh, yes, thanks for pointing that out. It would have been nice to
> > have caught that before v6.8 was released, but I guess it's better
> > than later.
> >
> >> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ...)
> >> now. Other two added lsm syscalls also have this issue.
> > Considering that Linux v6.8, and by extension these syscalls, are only
> > a few days old, I think I'd rather see us just modify the syscalls and
> > avoid the compat baggage. I'm going to be shocked if anyone has
> > shifted to using the new syscalls yet, and even if they have (!!),
> > moving from a "size_t" type to a "u64" should be mostly transparent
> > for the majority of native 64-bit systems. Those running the absolute
> > latest kernels on 32-bit systems with custom or bleeding edge
> > userspace *may* see a slight hiccup, but I think that user count is in
> > the single digits, if not zero.
> >
> > Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> > compat shim if we can.
> >
> > Casey, do you have time to put together a patch for this (you should
> > fix the call chains below the syscalls too)? If not, please let me
> > know and I'll get a patch out ASAP.
>
> Grumble. Yes, I'll get right on it.
Great, thanks Casey.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 05/11] LSM: Create lsm_list_modules system call
From: Casey Schaufler @ 2024-03-12 15:27 UTC (permalink / raw)
To: Paul Moore, Dmitry V. Levin
Cc: linux-security-module, jmorris, serge, keescook, john.johansen,
penguin-kernel, stephen.smalley.work, linux-kernel, linux-api,
mic, Casey Schaufler
In-Reply-To: <CAHC9VhRgjNT2YnVgCqMJnyr227qUjmfrWZ+LBnu_DGxnJZgeKw@mail.gmail.com>
On 3/12/2024 6:25 AM, Paul Moore wrote:
> On Tue, Mar 12, 2024 at 6:16 AM Dmitry V. Levin <ldv@strace.io> wrote:
>> On Tue, Sep 12, 2023 at 01:56:50PM -0700, Casey Schaufler wrote:
>> [...]
>>> --- a/security/lsm_syscalls.c
>>> +++ b/security/lsm_syscalls.c
>>> @@ -55,3 +55,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
>>> {
>>> return security_getselfattr(attr, ctx, size, flags);
>>> }
>>> +
>>> +/**
>>> + * sys_lsm_list_modules - Return a list of the active security modules
>>> + * @ids: the LSM module ids
>>> + * @size: pointer to size of @ids, updated on return
>>> + * @flags: reserved for future use, must be zero
>>> + *
>>> + * Returns a list of the active LSM ids. On success this function
>>> + * returns the number of @ids array elements. This value may be zero
>>> + * if there are no LSMs active. If @size is insufficient to contain
>>> + * the return data -E2BIG is returned and @size is set to the minimum
>>> + * required size. In all other cases a negative value indicating the
>>> + * error is returned.
>>> + */
>>> +SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, size_t __user *, size,
>>> + u32, flags)
>> I'm sorry but the size of userspace size_t is different from the kernel one
>> on 32-bit compat architectures.
> D'oh, yes, thanks for pointing that out. It would have been nice to
> have caught that before v6.8 was released, but I guess it's better
> than later.
>
>> Looks like there has to be a COMPAT_SYSCALL_DEFINE3(lsm_list_modules, ...)
>> now. Other two added lsm syscalls also have this issue.
> Considering that Linux v6.8, and by extension these syscalls, are only
> a few days old, I think I'd rather see us just modify the syscalls and
> avoid the compat baggage. I'm going to be shocked if anyone has
> shifted to using the new syscalls yet, and even if they have (!!),
> moving from a "size_t" type to a "u64" should be mostly transparent
> for the majority of native 64-bit systems. Those running the absolute
> latest kernels on 32-bit systems with custom or bleeding edge
> userspace *may* see a slight hiccup, but I think that user count is in
> the single digits, if not zero.
>
> Let's fix this quickly with /size_t/u64/ in v6.8.1 and avoid the
> compat shim if we can.
>
> Casey, do you have time to put together a patch for this (you should
> fix the call chains below the syscalls too)? If not, please let me
> know and I'll get a patch out ASAP.
Grumble. Yes, I'll get right on it.
>
> Thanks all.
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox