Re: [PATCH 1/2] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case
On Sat, Nov 02, 2024 at 11:29:55AM +, Zbigniew Jędrzejewski-Szmek wrote: > On Thu, Oct 31, 2024 at 03:10:37PM -0700, Kees Cook wrote: > > On Wed, 30 Oct 2024 14:37:31 -0600, Tycho Andersen wrote: > > > Zbigniew mentioned at Linux Plumber's that systemd is interested in > > > switching to execveat() for service execution, but can't, because the > > > contents of /proc/pid/comm are the file descriptor which was used, > > > instead of the path to the binary. This makes the output of tools like > > > top and ps useless, especially in a world where most fds are opened > > > CLOEXEC so the number is truly meaningless. > > > > > > [...] > > > > Applied to for-next/execve, thanks! > > > > [1/2] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case > > https://git.kernel.org/kees/c/7bdc6fc85c9a > > [2/2] selftests/exec: add a test for execveat()'s comm > > https://git.kernel.org/kees/c/bd104872311a > > I tested this with systemd compiled with -Dfexece=true and it all > seems to work fine. Thanks! Great; thank you! -- Kees Cook
Re: [PATCH 1/2] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case
On Wed, 30 Oct 2024 14:37:31 -0600, Tycho Andersen wrote: > Zbigniew mentioned at Linux Plumber's that systemd is interested in > switching to execveat() for service execution, but can't, because the > contents of /proc/pid/comm are the file descriptor which was used, > instead of the path to the binary. This makes the output of tools like > top and ps useless, especially in a world where most fds are opened > CLOEXEC so the number is truly meaningless. > > [...] Applied to for-next/execve, thanks! [1/2] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case https://git.kernel.org/kees/c/7bdc6fc85c9a [2/2] selftests/exec: add a test for execveat()'s comm https://git.kernel.org/kees/c/bd104872311a Take care, -- Kees Cook
Re: [PATCH v2 1/2] exec: add a flag for "reasonable" execveat() comm
On Fri, Sep 27, 2024 at 10:45:58AM -0500, Eric W. Biederman wrote:
> Tycho Andersen writes:
>
> > From: Tycho Andersen
> >
> > Zbigniew mentioned at Linux Plumber's that systemd is interested in
> > switching to execveat() for service execution, but can't, because the
> > contents of /proc/pid/comm are the file descriptor which was used,
> > instead of the path to the binary. This makes the output of tools like
> > top and ps useless, especially in a world where most fds are opened
> > CLOEXEC so the number is truly meaningless.
> >
> > Change exec path to fix up /proc/pid/comm in the case where we have
> > allocated one of these synthetic paths in bprm_init(). This way the actual
> > exec machinery is unchanged, but cosmetically the comm looks reasonable to
> > admins investigating things.
>
> Perhaps change the subject to match the code.
>
> > Signed-off-by: Tycho Andersen
> > Suggested-by: Zbigniew Jędrzejewski-Szmek
> > CC: Aleksa Sarai
> > Link:
> > https://github.com/uapi-group/kernel-features#set-comm-field-before-exec
> > ---
> > v2: * drop the flag, everyone :)
> > * change the rendered value to f_path.dentry->d_name.name instead of
> > argv[0], Eric
> > ---
> > fs/exec.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index dad402d55681..9520359a8dcc 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1416,7 +1416,18 @@ int begin_new_exec(struct linux_binprm * bprm)
> > set_dumpable(current->mm, SUID_DUMP_USER);
> >
> > perf_event_exec();
> > - __set_task_comm(me, kbasename(bprm->filename), true);
> > +
> > + /*
> > +* If fdpath was set, execveat() made up a path that will
> > +* probably not be useful to admins running ps or similar.
> > +* Let's fix it up to be something reasonable.
> > +*/
> > + if (bprm->fdpath) {
> > + BUILD_BUG_ON(TASK_COMM_LEN > DNAME_INLINE_LEN);
> > + __set_task_comm(me, bprm->file->f_path.dentry->d_name.name,
> > true);
>
> We can just do this regardless of bprm->fdpath.
>
> It will be a change of behavior on when executing symlinks and possibly
> mount points but I don't think we care. If we do then we can add make
> it conditional with "if (bprm->fdpath)"
>
> At the very least using the above version unconditionally ought to flush
> out any bugs.
I'm not super comfortable doing this regardless of bprm->fdpath; that
seems like too many cases getting changed. Can we just leave it as
depending on bprm->fdpath?
Also, is d_name.name always going to be set? e.g. what about memfd, etc?
--
Kees Cook
Re: [PATCH] kunit: Fix missing kerneldoc comment
On Thu, Sep 05, 2024 at 10:47:55AM +0800, David Gow wrote:
> Add a missing kerneldoc comment for the 'test' test context parameter,
> fixing the following warning:
>
> include/kunit/test.h:492: warning: Function parameter or struct member 'test'
> not described in 'kunit_kfree_const'
>
> Reported-by: Stephen Rothwell
> Closes: https://lore.kernel.org/lkml/20240827160631.67e12...@canb.auug.org.au/
> Fixes: f2c6dbd22017 ("kunit: Device wrappers should also manage driver name")
> Signed-off-by: David Gow
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG
On Wed, 07 Aug 2024 15:05:11 -0700, Song Liu wrote: > With CONFIG_LTO_CLANG, the compiler/linker adds .llvm. suffix to > local symbols to avoid duplications. Existing scripts/kallsyms sorts > symbols without .llvm. suffix. However, this causes quite some > issues later on. Some users of kallsyms, such as livepatch, have to match > symbols exactly. > > Address this by sorting full symbols at build time, and let kallsyms > lookup APIs to match the symbols exactly. > > [...] Applied to for-linus/hardening, thanks! [1/2] kallsyms: Do not cleanup .llvm. suffix before sorting symbols https://git.kernel.org/kees/c/020925ce9299 [2/2] kallsyms: Match symbols exactly with CONFIG_LTO_CLANG https://git.kernel.org/kees/c/fb6a421fb615 Take care, -- Kees Cook
Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG
On Mon, Aug 12, 2024 at 06:13:22PM +, Song Liu wrote: > Hi Luis, > > > On Aug 12, 2024, at 9:57 AM, Luis Chamberlain wrote: > > > > On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote: > >> Hi folks, > >> > >> Do we have more concerns and/or suggestions with this set? If not, > >> what would be the next step for it? > > > > I'm all for simplifying things, and this does just that, however, > > I'm not the one you need to convince, the folks who added the original > > hacks should provide their Reviewed-by / Tested-by not just for > > CONFIG_LTO_CLANG > > but also given this provides an alternative fix, don't we want to invert > > the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches > > also have their respective Fixes tag? > > kallsyms has got quite a few changes/improvements in the past few years: > > 1. Sami added logic to trim LTO hash in 2021 [1]; > 2. Zhen added logic to sort kallsyms in 2022 [2]; > 3. Yonghong changed cleanup_symbol_name() in 2023 [3]. > > In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes > tag to [1] or [3]? The patch won't apply to a kernel with only [1] > (without [2] and [3]); while this set is not just fixing [3]. So I think > it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG > and livepatching is probably not used by a lot of users, so I guess we > are OK without Fixes tags? I personally don't have a strong preference > either way. > > It is not necessary to invert the order of the two patches. Only applying > one of the two patches won't cause more issues than what we have today. Which tree should carry this series? -- Kees Cook
Re: [PATCHv2 bpf-next 1/9] uprobe: Add support for session consumer
On Fri, Jul 05, 2024 at 09:10:36AM +0200, Peter Zijlstra wrote: > On Wed, Jul 03, 2024 at 01:36:19PM -0700, Kees Cook wrote: > > > Yes, please use struct_size_t(). This is exactly what it was designed for. > > Kees, please, just let up, not going to happen. I'm getting really fed > up having to endlessly repeat what a piece of shite struct_size() is. I mean, okay, but the wrapper in the patch is basically the same thing. *shrug* > Put your time and effort into doing a proper language extension so we > can go and delete all that __builtin_*_overflow() based garbage. We are! That's in the future. Today, we have a saturating wrapper that provides type checking for the calculation's operands, and is in common use through-out the kernel. These are all things that the open-coded does not provide, so I continue to see it as an improvement over what else is available right now. I got asked for my opinion about whether to use struct_size() or not. In my opinion, this is a good place for it. I know you don't agree with me, but that wasn't the question. :) -Kees -- Kees Cook
Re: [PATCHv2 bpf-next 1/9] uprobe: Add support for session consumer
On Wed, Jul 03, 2024 at 11:31:11AM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 3, 2024 at 1:10 AM Peter Zijlstra wrote:
> >
> > On Tue, Jul 02, 2024 at 01:51:28PM -0700, Andrii Nakryiko wrote:
> > > > +static size_t ri_size(int sessions_cnt)
> > > > +{
> > > > + struct return_instance *ri __maybe_unused;
> > > > +
> > > > + return sizeof(*ri) + sessions_cnt * sizeof(ri->sessions[0]);
> > >
> > > just use struct_size()?
> >
> > Yeah, lets not. This is readable, struct_size() is not.
>
> This hack with __maybe_unused is more readable than the standard
> struct_size() helper that was added specifically for cases like this,
> really?
>
> I wonder if Kees agrees and whether there are any downsides to using
> struct_size()
>
> struct_size(struct return_instance, sessions, sessions_cnt) seems
> readable enough to me, in any case.
Yes, please use struct_size_t(). This is exactly what it was designed for.
Though with only 2 instances of ri_size(), maybe just use struct_size()
directly?
Also, please annotate struct return_instance with __counted_by:
+ int sessions_cnt;
+ struct session_consumer sessions[] __counted_by(sessions_cnt);
--
Kees Cook
Re: [PATCH v2] KUnit: add missing MODULE_DESCRIPTION() macros for lib/test_*.ko
On Wed, Jun 19, 2024 at 01:59:15PM -0700, Jeff Johnson wrote: > make allmodconfig && make W=1 C=1 reports for lib/test_*.ko: > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_hexdump.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_dhry.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_firmware.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_sysctl.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_hash.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_ida.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_list_sort.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_min_heap.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_module.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_sort.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_static_keys.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_static_key_base.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_memcat_p.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_blackhole_dev.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_meminit.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_free_pages.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_kprobes.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_ref_tracker.o > WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_bits.o > > Add the missing invocations of the MODULE_DESCRIPTION() macro. > > Signed-off-by: Jeff Johnson Thanks for chasing these down! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2] tracing: Add sched_prepare_exec tracepoint
On Thu, 11 Apr 2024 12:20:57 +0200, Marco Elver wrote: > Add "sched_prepare_exec" tracepoint, which is run right after the point > of no return but before the current task assumes its new exec identity. > > Unlike the tracepoint "sched_process_exec", the "sched_prepare_exec" > tracepoint runs before flushing the old exec, i.e. while the task still > has the original state (such as original MM), but when the new exec > either succeeds or crashes (but never returns to the original exec). > > [...] Applied to for-next/execve, thanks! [1/1] tracing: Add sched_prepare_exec tracepoint https://git.kernel.org/kees/c/5c5fad46e48c Take care, -- Kees Cook
Re: [PATCH v2] tracing: Add sched_prepare_exec tracepoint
On Thu, Apr 11, 2024 at 12:20:57PM +0200, Marco Elver wrote:
> Add "sched_prepare_exec" tracepoint, which is run right after the point
> of no return but before the current task assumes its new exec identity.
>
> Unlike the tracepoint "sched_process_exec", the "sched_prepare_exec"
> tracepoint runs before flushing the old exec, i.e. while the task still
> has the original state (such as original MM), but when the new exec
> either succeeds or crashes (but never returns to the original exec).
>
> Being able to trace this event can be helpful in a number of use cases:
>
> * allowing tracing eBPF programs access to the original MM on exec,
> before current->mm is replaced;
> * counting exec in the original task (via perf event);
> * profiling flush time ("sched_prepare_exec" to "sched_process_exec").
>
> Example of tracing output:
>
> $ cat /sys/kernel/debug/tracing/trace_pipe
> <...>-379 [003] . 179.626921: sched_prepare_exec:
> interp=/usr/bin/sshd filename=/usr/bin/sshd pid=379 comm=sshd
> <...>-381 [002] . 180.048580: sched_prepare_exec: interp=/bin/bash
> filename=/bin/bash pid=381 comm=sshd
> <...>-385 [001] . 180.068277: sched_prepare_exec:
> interp=/usr/bin/tty filename=/usr/bin/tty pid=385 comm=bash
> <...>-389 [006] . 192.020147: sched_prepare_exec:
> interp=/usr/bin/dmesg filename=/usr/bin/dmesg pid=389 comm=bash
>
> Signed-off-by: Marco Elver
This looks good to me. If tracing wants to take it:
Acked-by: Kees Cook
If not, I can take it in my tree if I get a tracing Ack. :)
-Kees
--
Kees Cook
Re: [PATCH] tracing: Add new_exec tracepoint
On Tue, Apr 09, 2024 at 08:25:45PM +0200, Marco Elver wrote:
> On Tue, Apr 09, 2024 at 08:46AM -0700, Kees Cook wrote:
> [...]
> > > + trace_new_exec(current, bprm);
> > > +
> >
> > All other steps in this function have explicit comments about
> > what/why/etc. Please add some kind of comment describing why the
> > tracepoint is where it is, etc.
>
> I beefed up the tracepoint documentation, and wrote a little paragraph
> above where it's called to reinforce what we want.
>
> [...]
> > What about binfmt_misc, and binfmt_script? You may want bprm->interp
> > too?
>
> Good points. I'll make the below changes for v2:
>
> diff --git a/fs/exec.c b/fs/exec.c
> index ab778ae1fc06..472b9f7b40e8 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1268,6 +1268,12 @@ int begin_new_exec(struct linux_binprm * bprm)
> if (retval)
> return retval;
>
> + /*
> + * This tracepoint marks the point before flushing the old exec where
> + * the current task is still unchanged, but errors are fatal (point of
> + * no return). The later "sched_process_exec" tracepoint is called after
> + * the current task has successfully switched to the new exec.
> + */
> trace_new_exec(current, bprm);
>
> /*
> diff --git a/include/trace/events/task.h b/include/trace/events/task.h
> index 8853dc44783d..623d9af777c1 100644
> --- a/include/trace/events/task.h
> +++ b/include/trace/events/task.h
> @@ -61,8 +61,11 @@ TRACE_EVENT(task_rename,
> * @task:pointer to the current task
> * @bprm:pointer to linux_binprm used for new exec
> *
> - * Called before flushing the old exec, but at the point of no return during
> - * switching to the new exec.
> + * Called before flushing the old exec, where @task is still unchanged, but
> at
> + * the point of no return during switching to the new exec. At the point it
> is
> + * called the exec will either succeed, or on failure terminate the task.
> Also
> + * see the "sched_process_exec" tracepoint, which is called right after @task
> + * has successfully switched to the new exec.
> */
> TRACE_EVENT(new_exec,
>
> @@ -71,19 +74,22 @@ TRACE_EVENT(new_exec,
> TP_ARGS(task, bprm),
>
> TP_STRUCT__entry(
> + __string( interp, bprm->interp)
> __string( filename, bprm->filename )
> __field(pid_t, pid )
> __string( comm, task->comm )
> ),
>
> TP_fast_assign(
> + __assign_str(interp, bprm->interp);
> __assign_str(filename, bprm->filename);
> __entry->pid = task->pid;
> __assign_str(comm, task->comm);
> ),
>
> - TP_printk("filename=%s pid=%d comm=%s",
> - __get_str(filename), __entry->pid, __get_str(comm))
> + TP_printk("interp=%s filename=%s pid=%d comm=%s",
> + __get_str(interp), __get_str(filename),
> + __entry->pid, __get_str(comm))
> );
>
> #endif
Looks good; I await v2, and Steven's Ack. :)
--
Kees Cook
Re: [PATCH] tracing: Add new_exec tracepoint
On Mon, Apr 08, 2024 at 11:01:54AM +0200, Marco Elver wrote:
> Add "new_exec" tracepoint, which is run right after the point of no
> return but before the current task assumes its new exec identity.
>
> Unlike the tracepoint "sched_process_exec", the "new_exec" tracepoint
> runs before flushing the old exec, i.e. while the task still has the
> original state (such as original MM), but when the new exec either
> succeeds or crashes (but never returns to the original exec).
>
> Being able to trace this event can be helpful in a number of use cases:
>
> * allowing tracing eBPF programs access to the original MM on exec,
> before current->mm is replaced;
> * counting exec in the original task (via perf event);
> * profiling flush time ("new_exec" to "sched_process_exec").
>
> Example of tracing output ("new_exec" and "sched_process_exec"):
>
> $ cat /sys/kernel/debug/tracing/trace_pipe
> <...>-379 [003] . 179.626921: new_exec:
> filename=/usr/bin/sshd pid=379 comm=sshd
> <...>-379 [003] . 179.629131: sched_process_exec:
> filename=/usr/bin/sshd pid=379 old_pid=379
> <...>-381 [002] . 180.048580: new_exec: filename=/bin/bash
> pid=381 comm=sshd
> <...>-381 [002] . 180.053122: sched_process_exec:
> filename=/bin/bash pid=381 old_pid=381
> <...>-385 [001] . 180.068277: new_exec: filename=/usr/bin/tty
> pid=385 comm=bash
> <...>-385 [001] . 180.069485: sched_process_exec:
> filename=/usr/bin/tty pid=385 old_pid=385
> <...>-389 [006] . 192.020147: new_exec:
> filename=/usr/bin/dmesg pid=389 comm=bash
>bash-389 [006] . 192.021377: sched_process_exec:
> filename=/usr/bin/dmesg pid=389 old_pid=389
>
> Signed-off-by: Marco Elver
> ---
> fs/exec.c | 2 ++
> include/trace/events/task.h | 30 ++
> 2 files changed, 32 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 38bf71cbdf5e..ab778ae1fc06 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1268,6 +1268,8 @@ int begin_new_exec(struct linux_binprm * bprm)
> if (retval)
> return retval;
>
> + trace_new_exec(current, bprm);
> +
All other steps in this function have explicit comments about
what/why/etc. Please add some kind of comment describing why the
tracepoint is where it is, etc.
For example, maybe something like:
/*
* Before any changes to 'current', report that the exec is about to
* happen (since we made it to the point of no return). On a successful
* exec, the 'sched_process_exec' tracepoint will also fire. On failure,
* ... [something else]
*/
> +TRACE_EVENT(new_exec,
> +
> + TP_PROTO(struct task_struct *task, struct linux_binprm *bprm),
> +
> + TP_ARGS(task, bprm),
> +
> + TP_STRUCT__entry(
> + __string( filename, bprm->filename )
> + __field(pid_t, pid )
> + __string( comm, task->comm )
> + ),
> +
> + TP_fast_assign(
> + __assign_str(filename, bprm->filename);
What about binfmt_misc, and binfmt_script? You may want bprm->interp
too?
-Kees
> + __entry->pid = task->pid;
> + __assign_str(comm, task->comm);
> + ),
> +
> + TP_printk("filename=%s pid=%d comm=%s",
> + __get_str(filename), __entry->pid, __get_str(comm))
> +);
> +
> #endif
>
> /* This part must be outside protection */
> --
> 2.44.0.478.gd926399ef9-goog
>
--
Kees Cook
Re: [PATCH 0/8] tracing: Persistent traces across a reboot or crash
On Sat, Mar 09, 2024 at 01:51:16PM -0500, Steven Rostedt wrote:
> On Sat, 9 Mar 2024 10:27:47 -0800
> Kees Cook wrote:
>
> > On Tue, Mar 05, 2024 at 08:59:10PM -0500, Steven Rostedt wrote:
> > > This is a way to map a ring buffer instance across reboots.
> >
> > As mentioned on Fedi, check out the persistent storage subsystem
> > (pstore)[1]. It already does what you're starting to construct for RAM
> > backends (but also supports reed-solomon ECC), and supports several
> > other backends including EFI storage (which is default enabled on at
> > least Fedora[2]), block devices, etc. It has an existing mechanism for
> > handling reservations (including via device tree), and supports multiple
> > "frontends" including the Oops handler, console output, and even ftrace
> > which does per-cpu recording and event reconstruction (Joel wrote this
> > frontend).
>
> Mathieu was telling me about the pmem infrastructure.
I use nvdimm to back my RAM backend testing with qemu so I can examine
the storage "externally":
RAM_SIZE=16384
NVDIMM_SIZE=200
MAX_SIZE=$(( RAM_SIZE + NVDIMM_SIZE ))
...
qemu-system-x86_64 \
...
-machine pc,nvdimm=on \
-m ${RAM_SIZE}M,slots=2,maxmem=${MAX_SIZE}M \
-object
memory-backend-file,id=mem1,share=on,mem-path=$IMAGES/x86/nvdimm.img,size=${NVDIMM_SIZE}M,align=128M
\
-device nvdimm,id=nvdimm1,memdev=mem1,label-size=1M \
...
-append 'console=uart,io,0x3f8,115200n8 loglevel=8 root=/dev/vda1 ro
ramoops.mem_size=1048576 ramoops.ecc=1 ramoops.mem_address=0x44000
ramoops.console_size=16384 ramoops.ftrace_size=16384 ramoops.pmsg_size=16384
ramoops.record_size=32768 panic=-1 init=/root/resume.sh '"$@"
The part I'd like to get wired up sanely is having pstore find the
nvdimm area automatically, but it never quite happened:
https://lore.kernel.org/lkml/CAGXu5jLtmb3qinZnX3rScUJLUFdf+pRDVPjy=cs4kutw9tl...@mail.gmail.com/
> Thanks for the info. We use pstore on ChromeOS, but it is currently
> restricted to 1MB which is too small for the tracing buffers. From what
> I understand, it's also in a specific location where there's only 1MB
> available for contiguous memory.
That's the area that is specifically hardware backed with persistent
RAM.
> I'm looking at finding a way to get consistent memory outside that
> range. That's what I'll be doing next week ;-)
>
> But this code was just to see if I could get a single contiguous range
> of memory mapped to ftrace, and this patch set does exactly that.
Well, please take a look at pstore. It should be able to do everything
you mention already; it just needs a way to define multiple regions if
you want to use an area outside of the persistent ram area defined by
Chrome OS's platform driver.
-Kees
--
Kees Cook
Re: [PATCH 0/8] tracing: Persistent traces across a reboot or crash
On Tue, Mar 05, 2024 at 08:59:10PM -0500, Steven Rostedt wrote: > This is a way to map a ring buffer instance across reboots. As mentioned on Fedi, check out the persistent storage subsystem (pstore)[1]. It already does what you're starting to construct for RAM backends (but also supports reed-solomon ECC), and supports several other backends including EFI storage (which is default enabled on at least Fedora[2]), block devices, etc. It has an existing mechanism for handling reservations (including via device tree), and supports multiple "frontends" including the Oops handler, console output, and even ftrace which does per-cpu recording and event reconstruction (Joel wrote this frontend). It should be pretty straight forward to implement a new frontend if the ftrace one isn't flexible enough. It's a bit clunky still to add one, but search for "ftrace" in fs/pstore/ram.c to see how to plumb a new frontend into the RAM backend. I continue to want to lift the frontend configuration options up into the pstore core, since it would avoid a bunch of redundancy, but this is where we are currently. :) -Kees [1] CONFIG_PSTORE et. al. in fs/pstore/ https://docs.kernel.org/admin-guide/ramoops.html [2] https://www.freedesktop.org/software/systemd/man/latest/systemd-pstore.service.html -- Kees Cook
Re: [kees:devel/overflow/sanitizers] [overflow] 660787b56e: UBSAN:signed-integer-overflow_in_lib/test_memcat_p.c
On Tue, Jan 30, 2024 at 10:52:56PM +0800, kernel test robot wrote:
>
>
> Hello,
>
> kernel test robot noticed
> "UBSAN:signed-integer-overflow_in_lib/test_memcat_p.c" on:
>
> commit: 660787b56e6e97ddc34c7882cbe1228f4040ef74 ("overflow: Reintroduce
> signed and unsigned overflow sanitizers")
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git
> devel/overflow/sanitizers
>
> in testcase: boot
>
> compiler: gcc-11
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
> we noticed this commit is reintroducing "signed and unsigned overflow
> sanitizers", there is below config diff between parent and this commit in our
> buildings:
>
> --- ea804316c9db5148d2bb0c1f40f70d7a83404638/.config2024-01-26
> 22:09:35.046768122 +0800
> +++ 660787b56e6e97ddc34c7882cbe1228f4040ef74/.config2024-01-26
> 19:53:20.693434428 +0800
> @@ -6706,6 +6706,7 @@ CONFIG_UBSAN_BOUNDS_STRICT=y
> CONFIG_UBSAN_SHIFT=y
> # CONFIG_UBSAN_DIV_ZERO is not set
> CONFIG_UBSAN_UNREACHABLE=y
> +CONFIG_UBSAN_SIGNED_WRAP=y
> # CONFIG_UBSAN_BOOL is not set
> # CONFIG_UBSAN_ENUM is not set
> # CONFIG_UBSAN_ALIGNMENT is not set
Hi! Thanks for the testing!
The added Kconfig is:
+config UBSAN_SIGNED_WRAP
+ bool "Perform checking for signed arithmetic wrap-around"
+ default UBSAN
+ depends on !COMPILE_TEST
+ depends on $(cc-option,-fsanitize=signed-integer-overflow)
So it looks like since your build already had CONFIG_UBSAN=y the signed
sanitizer got enabled too since it doesn't set CONFIG_COMPILE_TEST.
> while testing, we observed below different (and same part) between parent and
> this commit:
>
> ea804316c9db5148 660787b56e6e97ddc34c7882cbe
> ---
>fail:runs %reproductionfail:runs
>| | |
> 6:60% 6:6
> dmesg.UBSAN:shift-out-of-bounds_in_arch/x86/kernel/cpu/intel.c
> 6:60% 6:6
> dmesg.UBSAN:shift-out-of-bounds_in_arch/x86/kernel/cpu/topology.c
> 6:60% 6:6
> dmesg.UBSAN:shift-out-of-bounds_in_fs/namespace.c
> 6:60% 6:6
> dmesg.UBSAN:shift-out-of-bounds_in_fs/read_write.c
> 6:60% 6:6
> dmesg.UBSAN:shift-out-of-bounds_in_include/linux/rhashtable.h
> 6:60% 6:6
> dmesg.UBSAN:shift-out-of-bounds_in_include/net/tcp.h
Are these shift-out-of-bounds warnings new?
>:6 100% 6:6
> dmesg.UBSAN:signed-integer-overflow_in_lib/test_memcat_p.c
This is new for sure, catching an issue you show below...
> this looks like the commit uncovered issue. but since it's hard for us to back
> port this commit to each commit while bisecting, we cannot capture the real
> first bad commit. not sure if this report could help somebody to investigate
> the real issue?
Yeah, I think there is an unexpected wrap-around in test_memcat_p.c:
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot
> | Closes:
> https://lore.kernel.org/oe-lkp/202401302219.db90a6d5-oliver.s...@intel.com
>
>
> [ 42.894536][T1] [ cut here ]
> [ 42.895474][T1] UBSAN: signed-integer-overflow in
> lib/test_memcat_p.c:47:10
> [ 42.897128][T1] 6570 * 725861 cannot be represented in type 'int'
I'm surprised to see the sanitizer catching anything here since the
kernel is built with -fno-strict-overflow, but regardless, I'll send a
patch...
-Kees
--
Kees Cook
Re: [PATCH] eventfs: Save directory inodes in the eventfs_inode structure
On Mon, Jan 22, 2024 at 03:27:48PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)"
>
> The eventfs inodes and directories are allocated when referenced. But this
> leaves the issue of keeping consistent inode numbers and the number is
> only saved in the inode structure itself. When the inode is no longer
> referenced, it can be freed. When the file that the inode was representing
> is referenced again, the inode is once again created, but the inode number
> needs to be the same as it was before.
>
> Just making the inode numbers the same for all files is fine, but that
> does not work with directories. The find command will check for loops via
> the inode number and having the same inode number for directories triggers:
>
> # find /sys/kernel/tracing
> find: File system loop detected;
> '/sys/kernel/debug/tracing/events/initcall/initcall_finish' is part of the
> same file system loop as
> '/sys/kernel/debug/tracing/events/initcall'.
> [..]
>
> Linus pointed out that the eventfs_inode structure ends with a single
> 32bit int, and on 64 bit machines, there's likely a 4 byte hole due to
> alignment. We can use this hole to store the inode number for the
> eventfs_inode. All directories in eventfs are represented by an
> eventfs_inode and that data structure can hold its inode number.
>
> That last int was also purposely placed at the end of the structure to
> prevent holes from within. Now that there's a 4 byte number to hold the
> inode, both the inode number and the last integer can be moved up in the
> structure for better cache locality, where the llist and rcu fields can be
> moved to the end as they are only used when the eventfs_inode is being
> deleted.
>
> Link:
> https://lore.kernel.org/all/CAMuHMdXKiorg-jiuKoZpfZyDJ3Ynrfb8=x+c7x0eewxn-yr...@mail.gmail.com/
>
> Reported-by: Geert Uytterhoeven
> Fixes: 53c41052ba31 ("eventfs: Have the inodes all for files and directories
> all be the same")
> Signed-off-by: Steven Rostedt (Google)
Since I reviewed the earlier patch, I will repeat here for the formal
one too. :) Thanks for avoiding the hashing!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time
On December 21, 2023 4:16:56 AM PST, Michael Ellerman
wrote:
>Cc +Kees
>
>Christophe Leroy writes:
>> Declaring rodata_enabled and mark_rodata_ro() at all time
>> helps removing related #ifdefery in C files.
>>
>> Signed-off-by: Christophe Leroy
>> ---
>> include/linux/init.h | 4
>> init/main.c | 21 +++--
>> 2 files changed, 7 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/linux/init.h b/include/linux/init.h
>> index 01b52c9c7526..d2b47be38a07 100644
>> --- a/include/linux/init.h
>> +++ b/include/linux/init.h
>> @@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
>>
>> extern struct file_system_type rootfs_fs_type;
>>
>> -#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
>> extern bool rodata_enabled;
>> -#endif
>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>> void mark_rodata_ro(void);
>> -#endif
>>
>> extern void (*late_time_init)(void);
>>
>> diff --git a/init/main.c b/init/main.c
>> index e24b0780fdff..807df08c501f 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
>> early_param("rodata", set_debug_rodata);
>> #endif
>>
>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>> static void mark_readonly(void)
>> {
>> -if (rodata_enabled) {
>> +if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {
I think this will break without rodata_enabled actual existing on other
architectures. (Only declaration was made visible, not the definition, which is
above here and still behind ifdefs?)
-Kees
>> /*
>> * load_module() results in W+X mappings, which are cleaned
>> * up with call_rcu(). Let's make sure that queued work is
>> @@ -1409,20 +1408,14 @@ static void mark_readonly(void)
>> rcu_barrier();
>> mark_rodata_ro();
>> rodata_test();
>> -} else
>> +} else if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
>> pr_info("Kernel memory protection disabled.\n");
>> +} else if (IS_ENABLED(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)) {
>> +pr_warn("Kernel memory protection not selected by kernel
>> config.\n");
>> +} else {
>> +pr_warn("This architecture does not have kernel memory
>> protection.\n");
>> +}
>> }
>> -#elif defined(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)
>> -static inline void mark_readonly(void)
>> -{
>> -pr_warn("Kernel memory protection not selected by kernel config.\n");
>> -}
>> -#else
>> -static inline void mark_readonly(void)
>> -{
>> -pr_warn("This architecture does not have kernel memory protection.\n");
>> -}
>> -#endif
>>
>> void __weak free_initmem(void)
>> {
>> --
>> 2.41.0
--
Kees Cook
Re: [PATCH v3 0/5] params: harden string ops and allocatio ops
On Mon, 20 Nov 2023 17:11:41 +0200, Andy Shevchenko wrote: > A couple of patches are for get the string ops, used in the module, > slightly harden. On top a few cleanups. > > Since the main part is rather hardening, I think the Kees' tree is > the best fit for the series. It also possible to route via Greg's > sysfs (driver core?), but I'm open for another option(s). > > [...] Applied to for-next/hardening, thanks! [1/5] params: Introduce the param_unknown_fn type https://git.kernel.org/kees/c/aa61d651412a [2/5] params: Do not go over the limit when getting the string length https://git.kernel.org/kees/c/e6c5b15619a2 [3/5] params: Use size_add() for kmalloc() https://git.kernel.org/kees/c/9a4a4b528bff [4/5] params: Sort headers https://git.kernel.org/kees/c/18bdb5a032e8 [5/5] params: Fix multi-line comment style https://git.kernel.org/kees/c/c62c9771b7d6 Take care, -- Kees Cook
Re: [PATCH] eventfs: Use ERR_CAST() in eventfs_create_events_dir()
On Wed, Oct 18, 2023 at 11:10:31AM -0700, Nathan Chancellor wrote:
> When building with clang and CONFIG_RANDSTRUCT_FULL=y, there is an error
> due to a cast in eventfs_create_events_dir():
>
> fs/tracefs/event_inode.c:734:10: error: casting from randomized structure
> pointer type 'struct dentry *' to 'struct eventfs_inode *'
> 734 | return (struct eventfs_inode *)dentry;
> |^
> 1 error generated.
>
> Use the ERR_CAST() function to resolve the error, as it was designed for
> this exact situation (casting an error pointer to another type).
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1947
> Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use
> eventfs_inode")
> Signed-off-by: Nathan Chancellor
Yes, please. That's the correct method to do such casts. Thanks!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH] KVM: deprecate KVM_WERROR in favor of general WERROR
On Fri, Oct 06, 2023 at 01:54:15PM -0700, Jakub Kicinski wrote: > Setting WERROR for random subsystems make life really hard > for subsystems which want to build-test their stuff with W=1. > WERROR for the entire kernel now exists and can be used > instead. W=1 people probably know how to deal with the global > W=1 already, tracking all per-subsystem WERRORs is too much... > > Link: > https://lore.kernel.org/all/0da9874b6e9fcbaaa5edeb345d7e2a7c859fc818.1696271334.git.thomas.lenda...@amd.com/ > Signed-off-by: Jakub Kicinski Yeah, best to have just the global option. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2 0/5] params: harden string ops and allocatio ops
On Mon, Oct 02, 2023 at 03:48:51PM +0300, Andy Shevchenko wrote: > A couple of patches are for get the string ops, used in the module, > slightly harden. On top a few cleanups. > > Since the main part is rather hardening, I think the Kees' tree is > the best fit for the series, but I'm open for another option(s). > > Changelog v2: > - dropped the s*printf() --> sysfs_emit() conversion as it revealed > an issue, i.e. reuse getters with non-page-aligned pointer, which > would be addressed separately > - added cover letter and clarified the possible route for the series > (Luis) > > Andy Shevchenko (5): > params: Introduce the param_unknown_fn type > params: Do not go over the limit when getting the string length > params: Use size_add() for kmalloc() > params: Sort headers > params: Fix multi-line comment style Seems like a nice bit of clean-up. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2 2/5] params: Do not go over the limit when getting the string length
On Mon, Oct 02, 2023 at 03:48:53PM +0300, Andy Shevchenko wrote:
> We can use strnlen() even on early stages and it prevents from
> going over the string boundaries in case it's already too long.
It makes sense to avoid calling strlen() multiple times. I don't have
much opinion one way or another about using strnlen() here, since we
know the string will be terminated.
-Kees
>
> Signed-off-by: Andy Shevchenko
> ---
> kernel/params.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 626fa8265932..f8e3c4139854 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
>
> int param_set_charp(const char *val, const struct kernel_param *kp)
> {
> - if (strlen(val) > 1024) {
> + size_t len, maxlen = 1024;
> +
> + len = strnlen(val, maxlen + 1);
> + if (len == maxlen + 1) {
> pr_err("%s: string parameter too long\n", kp->name);
> return -ENOSPC;
> }
> @@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct
> kernel_param *kp)
> /* This is a hack. We can't kmalloc in early boot, and we
>* don't need to; this mangled commandline is preserved. */
> if (slab_is_available()) {
> - *(char **)kp->arg = kmalloc_parameter(strlen(val)+1);
> + *(char **)kp->arg = kmalloc_parameter(len + 1);
> if (!*(char **)kp->arg)
> return -ENOMEM;
> strcpy(*(char **)kp->arg, val);
> @@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct
> kernel_param *kp)
> {
> const struct kparam_string *kps = kp->str;
>
> - if (strlen(val)+1 > kps->maxlen) {
> + if (strnlen(val, kps->maxlen) == kps->maxlen) {
> pr_err("%s: string doesn't fit in %u chars.\n",
> kp->name, kps->maxlen-1);
> return -ENOSPC;
> --
> 2.40.0.1.gaa8946217a0b
>
--
Kees Cook
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, Sep 27, 2023 at 03:19:16AM +, Justin Stitt wrote: > Add the "D:" type which behaves the same as "K:" but will only match > content present in a patch file. > > To illustrate: > > Imagine this entry in MAINTAINERS: > > NEW REPUBLIC > M: Han Solo > W: https://www.jointheresistance.org > D: \bstrncpy\b > > Our maintainer, Han, will only be added to the recipients if a patch > file is passed to get_maintainer (like what b4 does): > $ ./scripts/get_maintainer.pl 0004-some-change.patch > > If the above patch has a `strncpy` present in the subject, commit log or > diff then Han will be to/cc'd. > > However, in the event of a file from the tree given like: > $ ./scripts/get_maintainer.pl ./lib/string.c > > Han will not be noisily to/cc'd (like a K: type would in this > circumstance) > > Note that folks really shouldn't be using get_maintainer on tree files > anyways [1]. > > [1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/ As Greg suggested, please drop the above paragraph and link. Then this looks good to me. I would immediately want to send this patch too, so please feel free to add this to your series (and I bet many other hints on "git grep 'K:.\\b'" would want to switch from K: to D: too): diff --git a/MAINTAINERS b/MAINTAINERS index 5f18c6ba3c3c..830e10866acf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5057,7 +5057,7 @@ F:Documentation/kbuild/llvm.rst F: include/linux/compiler-clang.h F: scripts/Makefile.clang F: scripts/clang-tools/ -K: \b(?i:clang|llvm)\b +D: \b(?i:clang|llvm)\b CLK API M: Russell King @@ -8199,7 +8199,7 @@ F:lib/strcat_kunit.c F: lib/strscpy_kunit.c F: lib/test_fortify/* F: scripts/test_fortify.sh -K: \b__NO_FORTIFY\b +D: \b__NO_FORTIFY\b FPGA DFL DRIVERS M: Wu Hao @@ -11457,9 +11457,9 @@ F: include/linux/overflow.h F: include/linux/randomize_kstack.h F: kernel/configs/hardening.config F: mm/usercopy.c -K: \b(add|choose)_random_kstack_offset\b -K: \b__check_(object_size|heap_object)\b -K: \b__counted_by\b +D: \b(add|choose)_random_kstack_offset\b +D: \b__check_(object_size|heap_object)\b +D: \b__counted_by\b KERNEL JANITORS L: kernel-janit...@vger.kernel.org @@ -17354,7 +17354,7 @@ F: drivers/acpi/apei/erst.c F: drivers/firmware/efi/efi-pstore.c F: fs/pstore/ F: include/linux/pstore* -K: \b(pstore|ramoops) +D: \b(pstore|ramoops) PTP HARDWARE CLOCK SUPPORT M: Richard Cochran @@ -19302,8 +19302,8 @@ F: include/uapi/linux/seccomp.h F: kernel/seccomp.c F: tools/testing/selftests/kselftest_harness.h F: tools/testing/selftests/seccomp/* -K: \bsecure_computing -K: \bTIF_SECCOMP\b +D: \bsecure_computing +D: \bTIF_SECCOMP\b SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER M: Kamal Dasu -- Kees Cook
Re: [PATCH 1/3] MAINTAINERS: add documentation for D:
On Wed, Sep 27, 2023 at 03:19:14AM +, Justin Stitt wrote: > Document what "D:" does. > > This is more or less the same as what "K:" does but only works for patch > files. > > See [3/3] for more info and an illustrative example. > --- > MAINTAINERS | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index b19995690904..de68d2c0cf29 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -59,6 +59,9 @@ Descriptions of section entries and preferred order > matches patches or files that contain one or more of the words > printk, pr_info or pr_err > One regex pattern per line. Multiple K: lines acceptable. > + D: *Content regex* (perl extended) pattern match patches only. > + Usage same as K:. > + The "emphasis" tags here are used when rendering: https://docs.kernel.org/process/maintainers.html In this case, I assume "D" is inspired by "Diff", so perhaps reword this to get a proper emphasis hint, and add additional context: D: *Diff content regex* (perl extended) pattern match that applies only to patches and not entire files (e.g. when using the get_maintainers.pl script). -- Kees Cook
Re: [PATCH 0/3] get_maintainer: add patch-only keyword matching
On Wed, Sep 27, 2023 at 08:24:58AM -0700, Nick Desaulniers wrote: > On Tue, Sep 26, 2023 at 8:19 PM Justin Stitt wrote: > > > > This series aims to add "D:" which behaves exactly the same as "K:" but > > works only on patch files. > > > > The goal of this is to reduce noise when folks use get_maintainer on > > tree files as opposed to patches. This use case should be steered away > > from [1] but "D:" should help maintainers reduce noise in their inboxes > > regardless, especially when matching omnipresent keywords like [2]. In > > the event of [2] Kees would be to/cc'd from folks running get_maintainer > > on _any_ file containing "__counted_by". The number of these files is > > rising and I fear for his inbox as his goal, as I understand it, is to > > simply monitor the introduction of new __counted_by annotations to > > ensure accurate semantics. > > Something like this (whether this series or a different approach) > would be helpful to me as well; we use K: to get cc'ed on patches > mentioning clang or llvm, but our ML also then ends up getting cc'ed > on every follow up patch to most files. > > This is causing excessive posts on our ML. As a result, it's a > struggle to get folks to cc themselves to the ML, which puts the code > review burden on fewer people. > > Whether it's a new D: or refinement to the behavior of K:, I applaud > the effort. Hopefully we can find an approach that works for > everyone. Yes, please! I would use this immediately -- there are a bunch of places where pstore, strings, hardening, etc all want review if certain functions or structures are changed in a patch, but we're not maintainers of the files they appear in. > > Justin Stitt (3): > > MAINTAINERS: add documentation for D: > > get_maintainer: add patch-only pattern matching type Can we squash these two changes together, and then likely add some patches for moving things out of K: ? -- Kees Cook
Re: [PATCH] MAINTAINERS: add include/linux/module*.h to modules
On Wed, Sep 20, 2023 at 02:10:09PM -0700, Luis Chamberlain wrote: > Use glob include/linux/module*.h to capture all module changes. > > Suggested-by: Kees Cook > Signed-off-by: Luis Chamberlain Thanks! Reviewed-by: Kees Cook -- Kees Cook
[PATCH] x86/mm/dump_pagetables: Add SLAB_VIRTUAL knowledge
Add the markings for the SLAB_VIRTUAL area.
Cc: Matteo Rizzo
Cc: Jann Horn
Cc: Dave Hansen
Cc: Andy Lutomirski
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: x...@kernel.org
Cc: "H. Peter Anvin"
Signed-off-by: Kees Cook
---
This is on top of the SLAB_VIRTUAL series:
https://lore.kernel.org/all/20230915105933.495735-11-matteori...@google.com/
Feel free to collapse this into the x86 patch from the above series.
FYI, as expected, the kernel page table entries get way longer with
SLAB_VIRTUAL. :)
Without SLAB_VIRTUAL:
# wc -l /sys/kernel/debug/page_tables/kernel
1501 /sys/kernel/debug/page_tables/kernel
With SLAB_VIRTUAL:
# wc -l /sys/kernel/debug/page_tables/kernel
7549 /sys/kernel/debug/page_tables/kernel
---
arch/x86/mm/dump_pagetables.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index e1b599ecbbc2..b1fa68669e61 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -64,6 +64,9 @@ enum address_markers_idx {
KASAN_SHADOW_END_NR,
#endif
CPU_ENTRY_AREA_NR,
+#ifdef CONFIG_SLAB_VIRTUAL
+ SLAB_AREA_NR,
+#endif
#ifdef CONFIG_X86_ESPFIX64
ESPFIX_START_NR,
#endif
@@ -95,6 +98,9 @@ static struct addr_marker address_markers[] = {
[LDT_NR]= { 0UL,"LDT remap" },
#endif
[CPU_ENTRY_AREA_NR] = { CPU_ENTRY_AREA_BASE,"CPU entry Area" },
+#ifdef CONFIG_SLAB_VIRTUAL
+ [SLAB_AREA_NR] = { SLAB_BASE_ADDR, "Slab Area" },
+#endif
#ifdef CONFIG_X86_ESPFIX64
[ESPFIX_START_NR] = { ESPFIX_BASE_ADDR, "ESPfix Area", 16 },
#endif
--
2.34.1
Re: [PATCH] HID: uhid: refactor deprecated strncpy
On Fri, Sep 15, 2023 at 09:36:23AM +0200, David Rheinsberg wrote:
> Hi
>
> On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
> >> - /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> >> - len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> >> - strncpy(hid->name, ev->u.create2.name, len);
> >> - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> >> - strncpy(hid->phys, ev->u.create2.phys, len);
> >> - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> >> - strncpy(hid->uniq, ev->u.create2.uniq, len);
> >
> > ev->u.create2 is:
> > struct uhid_create2_req {
> > __u8 name[128];
> > __u8 phys[64];
> > __u8 uniq[64];
> > ...
> >
> > hid is:
> > struct hid_device { /* device report descriptor */
> > ...
> > char name[128]; /* Device name */
> > char phys[64]; /* Device physical location */
> > char uniq[64]; /* Device unique identifier (serial #) */
> >
> > So these "min" calls are redundant -- it wants to copy at most 1 less so
> > it can be %NUL terminated. Which is what strscpy() already does. And
> > source and dest are the same size, so we can't over-read source if it
> > weren't terminated (since strscpy won't overread like strlcpy).
>
> I *really* think we should keep the `min` calls. The compiler
> should already optimize them away, as both arguments are compile-time
> constants. There is no inherent reason why source and target are equal in
> size. Yes, it is unlikely to change, but I don't understand why we would
> want to implicitly rely on it, rather than make the compiler verify it for
> us. And `struct hid_device` is very much allowed to change in the future.
>
> As an alternative, you can use BUILD_BUG_ON() and verify both are equal in
> length.
If we can't depend on ev->u.create2.name/phys/uniq being %NUL-terminated,
we've already done the "min" calculations, and we've already got the
dest zeroed, then I suspect the thing to do is just use memcpy instead
of strncpy (or strscpy).
--
Kees Cook
[PATCH] ceph: Annotate struct ceph_osd_request with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ceph_osd_request.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Ilya Dryomov
Cc: Xiubo Li
Cc: Jeff Layton
Cc: ceph-de...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/linux/ceph/osd_client.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h
index bf9823956758..b8610e9d2471 100644
--- a/include/linux/ceph/osd_client.h
+++ b/include/linux/ceph/osd_client.h
@@ -278,7 +278,7 @@ struct ceph_osd_request {
int r_attempts;
u32 r_map_dne_bound;
- struct ceph_osd_req_op r_ops[];
+ struct ceph_osd_req_op r_ops[] __counted_by(r_num_ops);
};
struct ceph_request_redirect {
--
2.34.1
[PATCH] afs: Annotate struct afs_permits with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct afs_permits.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: David Howells
Cc: Marc Dionne
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
fs/afs/internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 07433a5349ca..469a717467a4 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -705,7 +705,7 @@ struct afs_permits {
refcount_t usage;
unsigned short nr_permits; /* Number of records */
boolinvalidated;/* Invalidated due to key
change */
- struct afs_permit permits[]; /* List of permits sorted by
key pointer */
+ struct afs_permit permits[] __counted_by(nr_permits); /* List
of permits sorted by key pointer */
};
/*
--
2.34.1
[PATCH] ceph: Annotate struct ceph_monmap with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ceph_monmap.
Additionally, since the element count member must be set before accessing
the annotated flexible array member, move its initialization earlier.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Ilya Dryomov
Cc: Xiubo Li
Cc: Jeff Layton
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: ceph-de...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/linux/ceph/mon_client.h | 2 +-
net/ceph/mon_client.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/ceph/mon_client.h b/include/linux/ceph/mon_client.h
index b658961156a0..7a9a40163c0f 100644
--- a/include/linux/ceph/mon_client.h
+++ b/include/linux/ceph/mon_client.h
@@ -19,7 +19,7 @@ struct ceph_monmap {
struct ceph_fsid fsid;
u32 epoch;
u32 num_mon;
- struct ceph_entity_inst mon_inst[];
+ struct ceph_entity_inst mon_inst[] __counted_by(num_mon);
};
struct ceph_mon_client;
diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index faabad6603db..f263f7e91a21 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -1136,6 +1136,7 @@ static int build_initial_monmap(struct ceph_mon_client
*monc)
GFP_KERNEL);
if (!monc->monmap)
return -ENOMEM;
+ monc->monmap->num_mon = num_mon;
for (i = 0; i < num_mon; i++) {
struct ceph_entity_inst *inst = &monc->monmap->mon_inst[i];
@@ -1147,7 +1148,6 @@ static int build_initial_monmap(struct ceph_mon_client
*monc)
inst->name.type = CEPH_ENTITY_TYPE_MON;
inst->name.num = cpu_to_le64(i);
}
- monc->monmap->num_mon = num_mon;
return 0;
}
--
2.34.1
[PATCH] ocfs2: Annotate struct ocfs2_slot_info with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ocfs2_slot_info.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Mark Fasheh
Cc: Joel Becker
Cc: Joseph Qi
Cc: ocfs2-de...@lists.linux.dev
Signed-off-by: Kees Cook
---
fs/ocfs2/slot_map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ocfs2/slot_map.c b/fs/ocfs2/slot_map.c
index da7718cef735..e544c704b583 100644
--- a/fs/ocfs2/slot_map.c
+++ b/fs/ocfs2/slot_map.c
@@ -37,7 +37,7 @@ struct ocfs2_slot_info {
unsigned int si_blocks;
struct buffer_head **si_bh;
unsigned int si_num_slots;
- struct ocfs2_slot si_slots[];
+ struct ocfs2_slot si_slots[] __counted_by(si_num_slots);
};
--
2.34.1
[PATCH] afs: Annotate struct afs_addr_list with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct afs_addr_list.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: David Howells
Cc: Marc Dionne
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
fs/afs/internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index da73b97e19a9..07433a5349ca 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -87,7 +87,7 @@ struct afs_addr_list {
enum dns_lookup_status status:8;
unsigned long failed; /* Mask of addrs that failed
locally/ICMP */
unsigned long responded; /* Mask of addrs that responded
*/
- struct sockaddr_rxrpc addrs[];
+ struct sockaddr_rxrpc addrs[] __counted_by(max_addrs);
#define AFS_MAX_ADDRESSES ((unsigned int)(sizeof(unsigned long) * 8))
};
--
2.34.1
[PATCH] NFS/flexfiles: Annotate struct nfs4_ff_layout_segment with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct nfs4_ff_layout_segment.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Trond Myklebust
Cc: Anna Schumaker
Cc: linux-...@vger.kernel.org
Signed-off-by: Kees Cook
---
fs/nfs/flexfilelayout/flexfilelayout.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h
b/fs/nfs/flexfilelayout/flexfilelayout.h
index 354a031c69b1..f84b3fb0 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -99,7 +99,7 @@ struct nfs4_ff_layout_segment {
u64 stripe_unit;
u32 flags;
u32 mirror_array_cnt;
- struct nfs4_ff_layout_mirror*mirror_array[];
+ struct nfs4_ff_layout_mirror*mirror_array[]
__counted_by(mirror_array_cnt);
};
struct nfs4_flexfile_layout {
--
2.34.1
[PATCH] nfs41: Annotate struct nfs4_file_layout_dsaddr with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
nfs4_file_layout_dsaddr.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Trond Myklebust
Cc: Anna Schumaker
Cc: "Gustavo A. R. Silva"
Cc: linux-...@vger.kernel.org
Signed-off-by: Kees Cook
---
fs/nfs/filelayout/filelayout.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/filelayout/filelayout.h b/fs/nfs/filelayout/filelayout.h
index aed0748fd6ec..c7bb5da93307 100644
--- a/fs/nfs/filelayout/filelayout.h
+++ b/fs/nfs/filelayout/filelayout.h
@@ -51,7 +51,7 @@ struct nfs4_file_layout_dsaddr {
u32 stripe_count;
u8 *stripe_indices;
u32 ds_num;
- struct nfs4_pnfs_ds *ds_list[];
+ struct nfs4_pnfs_ds *ds_list[] __counted_by(ds_num);
};
struct nfs4_filelayout_segment {
--
2.34.1
[PATCH] aio: Annotate struct kioctx_table with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct kioctx_table.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Benjamin LaHaise
Cc: Alexander Viro
Cc: Christian Brauner
Cc: linux-...@kvack.org
Cc: linux-fsde...@vger.kernel.org
Signed-off-by: Kees Cook
---
fs/aio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/aio.c b/fs/aio.c
index a4c2a6bac72c..f8589caef9c1 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -80,7 +80,7 @@ struct aio_ring {
struct kioctx_table {
struct rcu_head rcu;
unsignednr;
- struct kioctx __rcu *table[];
+ struct kioctx __rcu *table[] __counted_by(nr);
};
struct kioctx_cpu {
--
2.34.1
[PATCH] mtd: rawnand: ingenic: Annotate struct ingenic_nfc with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ingenic_nfc.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Paul Cercueil
Cc: Harvey Hunt
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: linux-m...@vger.kernel.org
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
index 6748226b8bd1..ce9ef4e65597 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
@@ -46,7 +46,7 @@ struct ingenic_nfc {
struct nand_controller controller;
unsigned int num_banks;
struct list_head chips;
- struct ingenic_nand_cs cs[];
+ struct ingenic_nand_cs cs[] __counted_by(num_banks);
};
struct ingenic_nand {
--
2.34.1
[PATCH] udf: Annotate struct udf_bitmap with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct udf_bitmap.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Jan Kara
Signed-off-by: Kees Cook
---
fs/udf/udf_sb.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 9af6ff7f9747..f9a60bc1abcf 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -86,7 +86,7 @@ struct udf_virtual_data {
struct udf_bitmap {
__u32 s_extPosition;
int s_nr_groups;
- struct buffer_head *s_block_bitmap[];
+ struct buffer_head *s_block_bitmap[] __counted_by(s_nr_groups);
};
struct udf_part_map {
--
2.34.1
[PATCH] mtd: rawnand: sunxi: Annotate struct sunxi_nand_chip with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct sunxi_nand_chip.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: Chen-Yu Tsai
Cc: Jernej Skrabec
Cc: Samuel Holland
Cc: Manuel Dipolt
Cc: linux-...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-su...@lists.linux.dev
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/sunxi_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/sunxi_nand.c
b/drivers/mtd/nand/raw/sunxi_nand.c
index 9abf38049d35..4ec17c8bce5a 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -197,7 +197,7 @@ struct sunxi_nand_chip {
u32 timing_cfg;
u32 timing_ctl;
int nsels;
- struct sunxi_nand_chip_sel sels[];
+ struct sunxi_nand_chip_sel sels[] __counted_by(nsels);
};
static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand)
--
2.34.1
[PATCH] mtd: rawnand: marvell: Annotate struct marvell_nand_chip with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct marvell_nand_chip.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/marvell_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/marvell_nand.c
b/drivers/mtd/nand/raw/marvell_nand.c
index b841a81cb128..a46698744850 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -348,7 +348,7 @@ struct marvell_nand_chip {
int addr_cyc;
int selected_die;
unsigned int nsels;
- struct marvell_nand_chip_sel sels[];
+ struct marvell_nand_chip_sel sels[] __counted_by(nsels);
};
static inline struct marvell_nand_chip *to_marvell_nand(struct nand_chip *chip)
--
2.34.1
[PATCH] mtd: cfi: Annotate struct cfi_private with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct cfi_private.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
include/linux/mtd/cfi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index d88bb56c18e2..947410faf9e2 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -287,7 +287,7 @@ struct cfi_private {
unsigned long chipshift; /* Because they're of the same type */
const char *im_name; /* inter_module name for cmdset_setup */
unsigned long quirks;
- struct flchip chips[]; /* per-chip data structure for each chip */
+ struct flchip chips[] __counted_by(numchips); /* per-chip data
structure for each chip */
};
uint32_t cfi_build_cmd_addr(uint32_t cmd_ofs,
--
2.34.1
[PATCH] mtd: rawnand: meson: Annotate struct meson_nfc_nand_chip with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct meson_nfc_nand_chip.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Liang Yang
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: Neil Armstrong
Cc: Kevin Hilman
Cc: Jerome Brunet
Cc: Martin Blumenstingl
Cc: linux-...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-amlo...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/meson_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index 25e3c1cb605e..378f28ce6a74 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -128,7 +128,7 @@ struct meson_nfc_nand_chip {
u8 *data_buf;
__le64 *info_buf;
u32 nsels;
- u8 sels[];
+ u8 sels[] __counted_by(nsels);
};
struct meson_nand_ecc {
--
2.34.1
[PATCH] mtd: rawnand: renesas: Annotate struct rnand_chip with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct rnand_chip.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: linux-...@lists.infradead.org
Cc: linux-renesas-...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/renesas-nand-controller.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c
b/drivers/mtd/nand/raw/renesas-nand-controller.c
index 589021ea9eb2..c9a01feff8df 100644
--- a/drivers/mtd/nand/raw/renesas-nand-controller.c
+++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
@@ -210,7 +210,7 @@ struct rnand_chip {
u32 tim_gen_seq1;
u32 tim_gen_seq2;
u32 tim_gen_seq3;
- struct rnand_chip_sel sels[];
+ struct rnand_chip_sel sels[] __counted_by(nsels);
};
struct rnandc {
--
2.34.1
[PATCH] mtd: rawnand: atmel: Annotate struct atmel_nand with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct atmel_nand.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Tudor Ambarus
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: Nicolas Ferre
Cc: Alexandre Belloni
Cc: Claudiu Beznea
Cc: linux-...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/atmel/nand-controller.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c
b/drivers/mtd/nand/raw/atmel/nand-controller.c
index 3f494f7c7ecb..4cb478bbee4a 100644
--- a/drivers/mtd/nand/raw/atmel/nand-controller.c
+++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
@@ -165,7 +165,7 @@ struct atmel_nand {
struct atmel_pmecc_user *pmecc;
struct gpio_desc *cdgpio;
int numcs;
- struct atmel_nand_cs cs[];
+ struct atmel_nand_cs cs[] __counted_by(numcs);
};
static inline struct atmel_nand *to_atmel_nand(struct nand_chip *chip)
--
2.34.1
[PATCH] mtd: rawnand: denali: Annotate struct denali_chip with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct denali_chip.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/mtd/nand/raw/denali.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/denali.h b/drivers/mtd/nand/raw/denali.h
index ac46eb7956ce..5f2fab022fc5 100644
--- a/drivers/mtd/nand/raw/denali.h
+++ b/drivers/mtd/nand/raw/denali.h
@@ -328,7 +328,7 @@ struct denali_chip {
struct nand_chip chip;
struct list_head node;
unsigned int nsels;
- struct denali_chip_sel sels[];
+ struct denali_chip_sel sels[] __counted_by(nsels);
};
/**
--
2.34.1
[PATCH] mtd: Annotate struct lpddr_private with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct lpddr_private.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Miquel Raynal
Cc: Richard Weinberger
Cc: Vignesh Raghavendra
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
include/linux/mtd/qinfo.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/mtd/qinfo.h b/include/linux/mtd/qinfo.h
index 2e3f43788d48..0421f12156b5 100644
--- a/include/linux/mtd/qinfo.h
+++ b/include/linux/mtd/qinfo.h
@@ -24,7 +24,7 @@ struct lpddr_private {
struct qinfo_chip *qinfo;
int numchips;
unsigned long chipshift;
- struct flchip chips[];
+ struct flchip chips[] __counted_by(numchips);
};
/* qinfo_query_info structure contains request information for
--
2.34.1
[PATCH] leds: qcom-lpg: Annotate struct lpg_led with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct lpg_led.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: Bjorn Andersson
Cc: "Uwe Kleine-König"
Cc: Douglas Anderson
Cc: Anjelique Melendez
Cc: linux-l...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/leds/rgb/leds-qcom-lpg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index df469aaa7e6e..7d93e02a030a 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -173,7 +173,7 @@ struct lpg_led {
struct led_classdev_mc mcdev;
unsigned int num_channels;
- struct lpg_channel *channels[];
+ struct lpg_channel *channels[] __counted_by(num_channels);
};
/**
--
2.34.1
[PATCH] leds: mt6370: Annotate struct mt6370_priv with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct mt6370_priv.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: Matthias Brugger
Cc: AngeloGioacchino Del Regno
Cc: Alice Chen
Cc: Jacek Anaszewski
Cc: ChiYuan Huang
Cc: ChiaEn Wu
Cc: kernel test robot
Cc: linux-l...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-media...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/leds/flash/leds-mt6370-flash.c | 2 +-
drivers/leds/rgb/leds-mt6370-rgb.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/flash/leds-mt6370-flash.c
b/drivers/leds/flash/leds-mt6370-flash.c
index 931067c8a75f..912d9d622320 100644
--- a/drivers/leds/flash/leds-mt6370-flash.c
+++ b/drivers/leds/flash/leds-mt6370-flash.c
@@ -81,7 +81,7 @@ struct mt6370_priv {
unsigned int fled_torch_used;
unsigned int leds_active;
unsigned int leds_count;
- struct mt6370_led leds[];
+ struct mt6370_led leds[] __counted_by(leds_count);
};
static int mt6370_torch_brightness_set(struct led_classdev *lcdev, enum
led_brightness level)
diff --git a/drivers/leds/rgb/leds-mt6370-rgb.c
b/drivers/leds/rgb/leds-mt6370-rgb.c
index bb62431efe83..448d0da11848 100644
--- a/drivers/leds/rgb/leds-mt6370-rgb.c
+++ b/drivers/leds/rgb/leds-mt6370-rgb.c
@@ -153,7 +153,7 @@ struct mt6370_priv {
const struct mt6370_pdata *pdata;
unsigned int leds_count;
unsigned int leds_active;
- struct mt6370_led leds[];
+ struct mt6370_led leds[] __counted_by(leds_count);
};
static const struct reg_field common_reg_fields[F_MAX_FIELDS] = {
--
2.34.1
[PATCH] leds: lm3697: Annotate struct lm3697 with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct lm3697.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: linux-l...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/leds/leds-lm3697.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index cfb8ac220db6..380d17a58fe9 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -89,7 +89,7 @@ struct lm3697 {
int bank_cfg;
int num_banks;
- struct lm3697_led leds[];
+ struct lm3697_led leds[] __counted_by(num_banks);
};
static const struct reg_default lm3697_reg_defs[] = {
--
2.34.1
[PATCH] leds: mt6360: Annotate struct mt6360_priv with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct mt6360_priv.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: Matthias Brugger
Cc: AngeloGioacchino Del Regno
Cc: Gene Chen
Cc: Jacek Anaszewski
Cc: Andy Shevchenko
Cc: linux-l...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-media...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/leds/flash/leds-mt6360.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/flash/leds-mt6360.c b/drivers/leds/flash/leds-mt6360.c
index 1af6c5898343..b70dc689b33f 100644
--- a/drivers/leds/flash/leds-mt6360.c
+++ b/drivers/leds/flash/leds-mt6360.c
@@ -91,7 +91,7 @@ struct mt6360_priv {
unsigned int fled_torch_used;
unsigned int leds_active;
unsigned int leds_count;
- struct mt6360_led leds[];
+ struct mt6360_led leds[] __counted_by(leds_count);
};
static int mt6360_mc_brightness_set(struct led_classdev *lcdev,
--
2.34.1
[PATCH] leds: gpio: Annotate struct gpio_leds_priv with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct gpio_leds_priv.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: linux-l...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/leds/leds-gpio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 7bfe40a6bfdd..a6597f0f3eb4 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -142,7 +142,7 @@ static int create_gpio_led(const struct gpio_led *template,
struct gpio_leds_priv {
int num_leds;
- struct gpio_led_data leds[];
+ struct gpio_led_data leds[] __counted_by(num_leds);
};
static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
--
2.34.1
[PATCH] leds: el15203000: Annotate struct el15203000 with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct el15203000.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: linux-l...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/leds/leds-el15203000.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
index 7e7b617bcd56..d40194a3029f 100644
--- a/drivers/leds/leds-el15203000.c
+++ b/drivers/leds/leds-el15203000.c
@@ -80,7 +80,7 @@ struct el15203000 {
struct spi_device *spi;
unsigned long delay;
size_t count;
- struct el15203000_led leds[];
+ struct el15203000_led leds[] __counted_by(count);
};
#define to_el15203000_led(d) container_of(d, struct el15203000_led, ldev)
--
2.34.1
[PATCH] leds: cr0014114: Annotate struct cr0014114 with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct cr0014114.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: linux-l...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/leds/leds-cr0014114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
index b33bca397ea6..c9914fc51f20 100644
--- a/drivers/leds/leds-cr0014114.c
+++ b/drivers/leds/leds-cr0014114.c
@@ -56,7 +56,7 @@ struct cr0014114 {
struct spi_device *spi;
u8 *buf;
unsigned long delay;
- struct cr0014114_ledleds[];
+ struct cr0014114_ledleds[] __counted_by(count);
};
static void cr0014114_calc_crc(u8 *buf, const size_t len)
--
2.34.1
[PATCH] leds: aw200xx: Annotate struct aw200xx with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct aw200xx.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Pavel Machek
Cc: Lee Jones
Cc: linux-l...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/leds/leds-aw200xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 691a743cc9b0..4d517cace3e7 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -112,7 +112,7 @@ struct aw200xx {
struct mutex mutex;
u32 num_leds;
u32 display_rows;
- struct aw200xx_led leds[];
+ struct aw200xx_led leds[] __counted_by(num_leds);
};
static ssize_t dim_show(struct device *dev, struct device_attribute *devattr,
--
2.34.1
Re: [PATCH 00/21] dmaengine: Annotate with __counted_by
Hi,
Just a ping on the series... how do these look to you, Vinod?
If you want I can carry them in my tree. Please let me know.
Thanks!
-Kees
On Thu, Aug 17, 2023 at 04:58:37PM -0700, Kees Cook wrote:
> Hi,
>
> This annotates several structures with the coming __counted_by attribute
> for bounds checking of flexible arrays at run-time. For more details, see
> commit dd06e72e68bc ("Compiler Attributes: Add __counted_by macro").
>
> Thanks!
>
> -Kees
>
> Kees Cook (21):
> dmaengine: apple-admac: Annotate struct admac_data with __counted_by
> dmaengine: at_hdmac: Annotate struct at_desc with __counted_by
> dmaengine: axi-dmac: Annotate struct axi_dmac_desc with __counted_by
> dmaengine: fsl-edma: Annotate struct fsl_edma_desc with __counted_by
> dmaengine: hisilicon: Annotate struct hisi_dma_dev with __counted_by
> dmaengine: moxart-dma: Annotate struct moxart_desc with __counted_by
> dmaengine: qcom: bam_dma: Annotate struct bam_async_desc with
> __counted_by
> dmaengine: sa11x0: Annotate struct sa11x0_dma_desc with __counted_by
> dmaengine: sf-pdma: Annotate struct sf_pdma with __counted_by
> dmaengine: sprd: Annotate struct sprd_dma_dev with __counted_by
> dmaengine: st_fdma: Annotate struct st_fdma_desc with __counted_by
> dmaengine: stm32-dma: Annotate struct stm32_dma_desc with __counted_by
> dmaengine: stm32-mdma: Annotate struct stm32_mdma_desc with
> __counted_by
> dmaengine: stm32-mdma: Annotate struct stm32_mdma_device with
> __counted_by
> dmaengine: tegra: Annotate struct tegra_dma_desc with __counted_by
> dmaengine: tegra210-adma: Annotate struct tegra_adma with __counted_by
> dmaengine: ti: edma: Annotate struct edma_desc with __counted_by
> dmaengine: ti: omap-dma: Annotate struct omap_desc with __counted_by
> dmaengine: uniphier-xdmac: Annotate struct uniphier_xdmac_desc with
> __counted_by
> dmaengine: uniphier-xdmac: Annotate struct uniphier_xdmac_device with
> __counted_by
> dmaengine: usb-dmac: Annotate struct usb_dmac_desc with __counted_by
>
> drivers/dma/apple-admac.c | 2 +-
> drivers/dma/at_hdmac.c | 2 +-
> drivers/dma/dma-axi-dmac.c | 5 ++---
> drivers/dma/fsl-edma-common.h | 2 +-
> drivers/dma/hisi_dma.c | 2 +-
> drivers/dma/moxart-dma.c | 5 ++---
> drivers/dma/qcom/bam_dma.c | 2 +-
> drivers/dma/sa11x0-dma.c | 6 +++---
> drivers/dma/sf-pdma/sf-pdma.h | 2 +-
> drivers/dma/sh/usb-dmac.c | 2 +-
> drivers/dma/sprd-dma.c | 2 +-
> drivers/dma/st_fdma.h | 2 +-
> drivers/dma/stm32-dma.c| 11 ---
> drivers/dma/stm32-mdma.c | 9 -
> drivers/dma/tegra186-gpc-dma.c | 2 +-
> drivers/dma/tegra210-adma.c| 2 +-
> drivers/dma/ti/edma.c | 2 +-
> drivers/dma/ti/omap-dma.c | 5 ++---
> drivers/dma/uniphier-xdmac.c | 8
> 19 files changed, 33 insertions(+), 40 deletions(-)
>
> --
> 2.34.1
>
--
Kees Cook
[PATCH] wifi: brcmfmac: Annotate struct brcmf_gscan_config with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct brcmf_gscan_config.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Arend van Spriel
Cc: Franky Lin
Cc: Hante Meuleman
Cc: Kalle Valo
Cc: "Gustavo A. R. Silva"
Cc: Hector Martin
Cc: Ryohei Kondo
Cc: Hans de Goede
Cc: linux-wirel...@vger.kernel.org
Cc: brcm80211-dev-list@broadcom.com
Cc: sha-cyfmac-dev-l...@infineon.com
Signed-off-by: Kees Cook
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
index bece26741d3a..6eef6bc430e2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -1209,7 +1209,7 @@ struct brcmf_gscan_config {
u8 count_of_channel_buckets;
u8 retry_threshold;
__le16 lost_ap_window;
- struct brcmf_gscan_bucket_config bucket[];
+ struct brcmf_gscan_bucket_config bucket[]
__counted_by(count_of_channel_buckets);
};
/**
--
2.34.1
[PATCH] wifi: ipw2x00: Annotate struct libipw_txb with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct libipw_txb.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Stanislav Yakovlev
Cc: Kalle Valo
Cc: linux-wirel...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/intel/ipw2x00/libipw.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw.h
b/drivers/net/wireless/intel/ipw2x00/libipw.h
index bec7bc273748..9065ca5b0208 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw.h
+++ b/drivers/net/wireless/intel/ipw2x00/libipw.h
@@ -488,7 +488,7 @@ struct libipw_txb {
u8 reserved;
u16 frag_size;
u16 payload_size;
- struct sk_buff *fragments[];
+ struct sk_buff *fragments[] __counted_by(nr_frags);
};
/* SWEEP TABLE ENTRIES NUMBER */
--
2.34.1
[PATCH] wifi: brcmfmac: firmware: Annotate struct brcmf_fw_request with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct brcmf_fw_request.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Arend van Spriel
Cc: Franky Lin
Cc: Hante Meuleman
Cc: Kalle Valo
Cc: Matthias Brugger
Cc: Hector Martin
Cc: "Alvin Šipraga"
Cc: Hans de Goede
Cc: linux-wirel...@vger.kernel.org
Cc: brcm80211-dev-list@broadcom.com
Cc: sha-cyfmac-dev-l...@infineon.com
Signed-off-by: Kees Cook
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
index 1266cbaee072..4002d326fd21 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
@@ -69,7 +69,7 @@ struct brcmf_fw_request {
u16 bus_nr;
u32 n_items;
const char *board_types[BRCMF_FW_MAX_BOARD_TYPES];
- struct brcmf_fw_item items[];
+ struct brcmf_fw_item items[] __counted_by(n_items);
};
struct brcmf_fw_name {
--
2.34.1
[PATCH] wifi: mt76: Annotate struct mt76_rx_tid with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct mt76_rx_tid.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Felix Fietkau
Cc: Lorenzo Bianconi
Cc: Ryder Lee
Cc: Shayne Chen
Cc: Sean Wang
Cc: Kalle Valo
Cc: Matthias Brugger
Cc: AngeloGioacchino Del Regno
Cc: linux-wirel...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-media...@lists.infradead.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/mediatek/mt76/mt76.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
b/drivers/net/wireless/mediatek/mt76/mt76.h
index e8757865a3d0..03ef617b1527 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -376,7 +376,7 @@ struct mt76_rx_tid {
u8 started:1, stopped:1, timer_pending:1;
- struct sk_buff *reorder_buf[];
+ struct sk_buff *reorder_buf[] __counted_by(size);
};
#define MT_TX_CB_DMA_DONE BIT(0)
--
2.34.1
[PATCH] wifi: wcn36xx: Annotate struct wcn36xx_hal_ind_msg with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct wcn36xx_hal_ind_msg.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Loic Poulain
Cc: Kalle Valo
Cc: wcn3...@lists.infradead.org
Cc: linux-wirel...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/ath/wcn36xx/smd.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h
b/drivers/net/wireless/ath/wcn36xx/smd.h
index cf15cde2a364..2c1ed9e570bf 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -47,7 +47,7 @@ struct wcn36xx_fw_msg_status_rsp {
struct wcn36xx_hal_ind_msg {
struct list_head list;
size_t msg_len;
- u8 msg[];
+ u8 msg[] __counted_by(msg_len);
};
struct wcn36xx;
--
2.34.1
[PATCH] md/md-linear: Annotate struct linear_conf with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct linear_conf. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Song Liu Cc: linux-r...@vger.kernel.org Signed-off-by: Kees Cook --- drivers/md/md-linear.c | 26 +- drivers/md/md-linear.h | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c index 71ac99646827..ae2826e9645b 100644 --- a/drivers/md/md-linear.c +++ b/drivers/md/md-linear.c @@ -69,6 +69,19 @@ static struct linear_conf *linear_conf(struct mddev *mddev, int raid_disks) if (!conf) return NULL; + /* +* conf->raid_disks is copy of mddev->raid_disks. The reason to +* keep a copy of mddev->raid_disks in struct linear_conf is, +* mddev->raid_disks may not be consistent with pointers number of +* conf->disks[] when it is updated in linear_add() and used to +* iterate old conf->disks[] earray in linear_congested(). +* Here conf->raid_disks is always consitent with number of +* pointers in conf->disks[] array, and mddev->private is updated +* with rcu_assign_pointer() in linear_addr(), such race can be +* avoided. +*/ + conf->raid_disks = raid_disks; + cnt = 0; conf->array_sectors = 0; @@ -112,19 +125,6 @@ static struct linear_conf *linear_conf(struct mddev *mddev, int raid_disks) conf->disks[i-1].end_sector + conf->disks[i].rdev->sectors; - /* -* conf->raid_disks is copy of mddev->raid_disks. The reason to -* keep a copy of mddev->raid_disks in struct linear_conf is, -* mddev->raid_disks may not be consistent with pointers number of -* conf->disks[] when it is updated in linear_add() and used to -* iterate old conf->disks[] earray in linear_congested(). -* Here conf->raid_disks is always consitent with number of -* pointers in conf->disks[] array, and mddev->private is updated -* with rcu_assign_pointer() in linear_addr(), such race can be -* avoided. -*/ - conf->raid_disks = raid_disks; - return conf; out: diff --git a/drivers/md/md-linear.h b/drivers/md/md-linear.h index 24e97db50ebb..5587eeedb882 100644 --- a/drivers/md/md-linear.h +++ b/drivers/md/md-linear.h @@ -12,6 +12,6 @@ struct linear_conf struct rcu_head rcu; sector_tarray_sectors; int raid_disks; /* a copy of mddev->raid_disks */ - struct dev_info disks[]; + struct dev_info disks[] __counted_by(raid_disks); }; #endif -- 2.34.1
[PATCH] usb: gadget: f_midi: Annotate struct f_midi with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct f_midi.
Additionally, since the element count member must be set before accessing
the annotated flexible array member, move its initialization earlier.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Greg Kroah-Hartman
Cc: John Keeping
Cc: Peter Chen
Cc: Hulk Robot
Cc: Allen Pais
Cc: Will McVicker
Cc: Davidlohr Bueso
Cc: Zhang Qilong
Cc: linux-...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/usb/gadget/function/f_midi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_midi.c
b/drivers/usb/gadget/function/f_midi.c
index 2d02f25f9597..033e347554db 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -99,7 +99,7 @@ struct f_midi {
unsigned int in_last_port;
unsigned char free_ref;
- struct gmidi_in_portin_ports_array[/* in_ports */];
+ struct gmidi_in_portin_ports_array[] __counted_by(in_ports);
};
static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -1349,6 +1349,7 @@ static struct usb_function *f_midi_alloc(struct
usb_function_instance *fi)
status = -ENOMEM;
goto setup_fail;
}
+ midi->in_ports = opts->in_ports;
for (i = 0; i < opts->in_ports; i++)
midi->in_ports_array[i].cable = i;
@@ -1359,7 +1360,6 @@ static struct usb_function *f_midi_alloc(struct
usb_function_instance *fi)
status = -ENOMEM;
goto midi_free;
}
- midi->in_ports = opts->in_ports;
midi->out_ports = opts->out_ports;
midi->index = opts->index;
midi->buflen = opts->buflen;
--
2.34.1
[PATCH] usb: gadget: f_fs: Annotate struct ffs_buffer with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ffs_buffer.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Greg Kroah-Hartman
Cc: John Keeping
Cc: Udipto Goswami
Cc: Linyu Yuan
Cc: linux-...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/usb/gadget/function/f_fs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 6e9ef35a43a7..af400d083777 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -202,7 +202,7 @@ struct ffs_epfile {
struct ffs_buffer {
size_t length;
char *data;
- char storage[];
+ char storage[] __counted_by(length);
};
/* ffs_io_data structure ***/
--
2.34.1
[PATCH] usb: Annotate struct urb_priv with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct urb_priv.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Alan Stern
Cc: Greg Kroah-Hartman
Cc: Mathias Nyman
Cc: linux-...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/usb/host/ohci.h | 2 +-
drivers/usb/host/xhci.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index aac6285b37f8..1aba22784e05 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -337,7 +337,7 @@ typedef struct urb_priv {
u16 length; // # tds in this request
u16 td_cnt; // tds already serviced
struct list_headpending;
- struct td *td[]; // all TDs in this request
+ struct td *td[] __counted_by(length); // all TDs in this
request
} urb_priv_t;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..2f21c3a8565c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1666,7 +1666,7 @@ struct xhci_scratchpad {
struct urb_priv {
int num_tds;
int num_tds_done;
- struct xhci_td td[];
+ struct xhci_td td[] __counted_by(num_tds);
};
/*
--
2.34.1
Re: [PATCH][next] net: spider_net: Use size_add() in call to struct_size()
On Fri, Sep 15, 2023 at 01:25:36PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound,
> the protection that `struct_size()` adds against potential integer
> overflows is defeated. Fix this by hardening call to `struct_size()`
> with `size_add()`.
>
> Fixes: 3f1071ec39f7 ("net: spider_net: Use struct_size() helper")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH][next] tipc: Use size_add() in calls to struct_size()
On Fri, Sep 15, 2023 at 01:16:26PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound,
> the protection that `struct_size()` adds against potential integer
> overflows is defeated. Fix this by hardening call to `struct_size()`
> with `size_add()`.
>
> Fixes: e034c6d23bc4 ("tipc: Use struct_size() helper")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH][next] usb: atm: Use size_add() in call to struct_size()
On Fri, Sep 15, 2023 at 01:20:14PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound,
> the protection that `struct_size()` adds against potential integer
> overflows is defeated. Fix this by hardening call to `struct_size()`
> with `size_add()`.
>
> Fixes: b626871a7cda ("usb: atm: Use struct_size() helper")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH][next] tls: Use size_add() in call to struct_size()
On Fri, Sep 15, 2023 at 01:12:38PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound,
> the protection that `struct_size()` adds against potential integer
> overflows is defeated. Fix this by hardening call to `struct_size()`
> with `size_add()`.
>
> Fixes: b89fec54fd61 ("tls: rx: wrap decrypt params in a struct")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH][next] mlxsw: Use size_mul() in call to struct_size()
On Fri, Sep 15, 2023 at 01:01:23PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound, the
> protection that `struct_size()` adds against potential integer overflows
> is defeated. Fix this by hardening call to `struct_size()` with `size_mul()`.
>
> Fixes: 2285ec872d9d ("mlxsw: spectrum_acl_bloom_filter: use struct_size() in
> kzalloc()")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH][next] ASoC: SOF: ipc4-topology: Use size_add() in call to struct_size()
On Fri, Sep 15, 2023 at 01:09:11PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound,
> the protection that `struct_size()` adds against potential integer
> overflows is defeated. Fix this by hardening call to `struct_size()`
> with `size_add()`.
>
> Fixes: f9efae954905 ("ASoC: SOF: ipc4-topology: Add support for base config
> extension")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v3 1/2] uapi: fix __DECLARE_FLEX_ARRAY for C++
On Tue, Sep 12, 2023 at 07:22:24PM +0300, Alexey Dobriyan wrote:
> __DECLARE_FLEX_ARRAY(T, member) macro expands to
>
> struct {
> struct {} __empty_member;
> T member[];
> };
>
> which is subtly wrong in C++ because sizeof(struct{}) is 1 not 0,
> changing UAPI structures layouts.
Looking at this again just now, what about using a 0-length array
instead of an anonymous struct?
https://godbolt.org/z/rGaxPWjef
Then we don't need an #ifdef at all...
struct {
int __empty_member[0];
T member[];
};
-Kees
--
Kees Cook
Re: [PATCH][next] gve: Use size_add() in call to struct_size()
On Fri, Sep 15, 2023 at 12:17:49PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, `tx_stats_num + rx_stats_num` wraps around, the
> protection that struct_size() adds against potential integer overflows
> is defeated. Fix this by hardening call to struct_size() with size_add().
>
> Fixes: 691f4077d560 ("gve: Replace zero-length array with flexible-array
> member")
> Signed-off-by: Gustavo A. R. Silva
Thanks, yes, this will maintain SIZE_MAX saturation if it happens.
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH] firmware: ti_sci: refactor deprecated strncpy
On Fri, Sep 15, 2023 at 07:40:38AM -0500, Nishanth Menon wrote: > On 21:03-20230914, Kees Cook wrote: > > On Wed, Sep 13, 2023 at 08:23:02PM +, Justin Stitt wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > > > We should prefer more robust and less ambiguous string interfaces. > > > > > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > > > NUL-termination on the destination buffer. > > > > > > It does not seem like `ver->firmware_description` requires NUL-padding > > > (which is a behavior that strncpy provides) but if it does let's opt for > > > `strscpy_pad()`. > > > > > > Link: > > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > > [1] > > > Link: > > > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > > Link: https://github.com/KSPP/linux/issues/90 > > > Cc: linux-harden...@vger.kernel.org > > > Signed-off-by: Justin Stitt > > > > Looks right to me. > > > > Reviewed-by: Kees Cook > > Does this belong to stable as well? If so, please add appropriate stable > process. No need. This is a refactoring only. :) -- Kees Cook
Re: [PATCH v4] kobject: Replace strlcpy with strscpy
On Thu, 31 Aug 2023 14:01:04 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Applied to for-next/hardening, thanks! [1/1] kobject: Replace strlcpy with strscpy https://git.kernel.org/kees/c/68a39dfd6f94 Take care, -- Kees Cook
Re: [PATCH] init/version.c: Replace strlcpy with strscpy
On Wed, 30 Aug 2023 16:08:06 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Applied to for-next/hardening, thanks! [1/1] init/version.c: Replace strlcpy with strscpy https://git.kernel.org/kees/c/ec23bc09c1c0 Take care, -- Kees Cook
Re: [PATCH] HID: uhid: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 10:47:21PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should prefer more robust and less ambiguous string interfaces.
>
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
>
> Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of
> strncpy()"")
> we see referenced the fact that many attempts have been made to change
> these strncpy's into strlcpy to no success. I think strscpy is an
> objectively better interface here as it doesn't unnecessarily NUL-pad
> the destination buffer whilst allowing us to drop the `len = min(...)`
> pattern as strscpy will implicitly limit the number of bytes copied by
> the smaller of its dest and src arguments.
I think the worry here is that ev->u.create2.name may not be %NUL
terminated. But I think that doesn't matter, see below...
Also, note, this should CC David Herrmann
(now added).
> So while the existing code may not have a bug (i.e: overread problems)
> we should still favor strscpy due to readability (plus a very slight
> performance boost).
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> drivers/hid/uhid.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 4588d2cd4ea4..00e1566ad37b 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
> const struct uhid_event *ev)
> {
> struct hid_device *hid;
> - size_t rd_size, len;
> + size_t rd_size;
> void *rd_data;
> int ret;
>
> @@ -514,13 +514,9 @@ static int uhid_dev_create2(struct uhid_device *uhid,
> goto err_free;
> }
>
> - /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> - len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> - strncpy(hid->name, ev->u.create2.name, len);
> - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> - strncpy(hid->phys, ev->u.create2.phys, len);
> - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> - strncpy(hid->uniq, ev->u.create2.uniq, len);
ev->u.create2 is:
struct uhid_create2_req {
__u8 name[128];
__u8 phys[64];
__u8 uniq[64];
...
hid is:
struct hid_device { /* device report descriptor */
...
char name[128]; /* Device name */
char phys[64]; /* Device physical location */
char uniq[64]; /* Device unique identifier (serial #) */
So these "min" calls are redundant -- it wants to copy at most 1 less so
it can be %NUL terminated. Which is what strscpy() already does. And
source and dest are the same size, so we can't over-read source if it
weren't terminated (since strscpy won't overread like strlcpy).
> + strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
> + strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
> + strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
Reviewed-by: Kees Cook
-Kees
>
> hid->ll_driver = &uhid_hid_driver;
> hid->bus = ev->u.create2.bus;
>
> ---
> base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
> change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] HID: prodikeys: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 10:20:55PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Note: for some reason if NUL-padding is needed let's opt for `strscpy_pad()` > --- > drivers/hid/hid-prodikeys.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c > index e4e9471d0f1e..c16d2ba6ea16 100644 > --- a/drivers/hid/hid-prodikeys.c > +++ b/drivers/hid/hid-prodikeys.c > @@ -639,9 +639,9 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) > goto fail; > } > > - strncpy(card->driver, shortname, sizeof(card->driver)); > - strncpy(card->shortname, shortname, sizeof(card->shortname)); > - strncpy(card->longname, longname, sizeof(card->longname)); > + strscpy(card->driver, shortname, sizeof(card->driver)); > + strscpy(card->shortname, shortname, sizeof(card->shortname)); > + strscpy(card->longname, longname, sizeof(card->longname)); "card" is already kzalloc()ed so no _pad() is needed, good. > > /* Set up rawmidi */ > err = snd_rawmidi_new(card, card->shortname, 0, > @@ -652,7 +652,7 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) > goto fail; > } > pm->rwmidi = rwmidi; > - strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); > + strscpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); > rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT; > rwmidi->private_data = pm; Same here. Reviewed-by: Kees Cook -Kees > > > --- > base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec > change-id: 20230914-strncpy-drivers-hid-hid-prodikeys-c-cf42614a21d4 > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH] firmware: ti_sci: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 08:23:02PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer. > > It does not seem like `ver->firmware_description` requires NUL-padding > (which is a behavior that strncpy provides) but if it does let's opt for > `strscpy_pad()`. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Looks right to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] firmware: tegra: bpmp: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 07:38:44PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > It seems like the filename stored at `namevirt` is expected to be > NUL-terminated. This took me a bit to establish, but yes: buf[256] is used to store filename, so it'll always be %NUL-terminated with the 256 bytes, which is the same size used to allocate virtname, which means it will always be %NUL-terminated. > > A suitable replacement is `strscpy_pad` due to the fact that it > guarantees NUL-termination on the destination buffer whilst maintaining > the NUL-padding behavior that strncpy provides. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt This one looks weird because namevirt seems unused, but I assume there's some kind of DMA side-effect happening somewhere? But, yes, after digging around here, I think this all looks right. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v3] EDAC/mc_sysfs: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 05:20:50PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer without needlessly > NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Changes in v3: > - prefer strscpy to strscpy_pad (thanks Tony) > - Link to v2: > https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v2-1-2d2e6bd43...@google.com > > Changes in v2: > - included refactor of another strncpy in same file > - Link to v1: > https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v1-1-d232891b0...@google.com > --- > Note: build-tested only. > --- > drivers/edac/edac_mc_sysfs.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > index 15f63452a9be..9a5b4bbd8191 100644 > --- a/drivers/edac/edac_mc_sysfs.c > +++ b/drivers/edac/edac_mc_sysfs.c > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device > *dev, > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > return -EINVAL; > > - strncpy(rank->dimm->label, data, copy_count); > - rank->dimm->label[copy_count] = '\0'; > + strscpy(rank->dimm->label, data, copy_count); Hrm, I don't like the use of "copy_count" here -- it's the source length, not the destination buffer size. It is technically safe because it was bounds-checked right before, but now we run the risk of additional truncation since "copy_count" will not include the '\0'. Imagine data = "a", count = 1. strscpy(dst, data, 1) will only copy '\0'. I think this should be memcpy(), not strscpy(). The bounds checking and truncation of '\n' has already been calculated -- we're just doing a byte copy at this point: if (count == 0) return -EINVAL; if (data[count - 1] == '\0' || data[count - 1] == '\n') copy_count -= 1; if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; memcpy(rank->dimm->label, data, copy_count); rank->dimm->label[copy_count] = '\0'; > > return count; > } > @@ -535,7 +534,7 @@ static ssize_t dimmdev_label_store(struct device *dev, > if (copy_count == 0 || copy_count >= sizeof(dimm->label)) > return -EINVAL; > > - strncpy(dimm->label, data, copy_count); > + strscpy(dimm->label, data, copy_count); > dimm->label[copy_count] = '\0'; Same for this one: replace strncpy with memcpy. -Kees > > return count; > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH] dax: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 01:10:24AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > `dax_id->dev_name` is expected to be NUL-terminated and has been > zero-allocated. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer. Moreover, due to > `dax_id` being zero-allocated the padding behavior of `strncpy` is not > needed and a simple 1:1 replacement of strncpy -> strscpy should > suffice. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Looks correct to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] cpuidle: dt: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 12:23:19AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer. With this, we can also drop > the now unnecessary `CPUIDLE_(NAME|DESC)_LEN - 1` pieces. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt A very regular strncpy/strscpy conversion. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] cpufreq: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 12:07:21AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > Both `policy->last_governor` and `default_governor` are expected to be > NUL-terminated which is shown by their heavy usage with other string > apis like `strcmp`. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt All looks sensible to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] bus: fsl-mc: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 10:52:04PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We need to prefer more robust and less ambiguous string interfaces. > > `obj_desc->(type|label)` are expected to be NUL-terminated strings as > per "include/linux/fsl/mc.h +143" > | ... > | * struct fsl_mc_obj_desc - Object descriptor > | * @type: Type of object: NULL terminated string > | ... > > It seems `cmd_params->obj_type` is also expected to be a NUL-terminated > string. > > A suitable replacement is `strscpy_pad` due to the fact that it > guarantees NUL-termination on the destination buffer whilst keeping the > NUL-padding behavior that `strncpy` provides. I see evidence that %NUL padding isn't needed, like this: /* * Mark the obj entry as "invalid", by using the * empty string as obj type: */ obj_desc->type[0] = '\0'; but there's little harm in being conservative and leaving the padding in. > > Padding may not strictly be necessary but let's opt to keep it as this > ensures no functional change. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Cc: Kees Cook > Signed-off-by: Justin Stitt > --- > Note: build-tested > --- > drivers/bus/fsl-mc/dprc.c | 12 > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c > index d129338b8bc0..dd1b5c0fb7e2 100644 > --- a/drivers/bus/fsl-mc/dprc.c > +++ b/drivers/bus/fsl-mc/dprc.c > @@ -450,10 +450,8 @@ int dprc_get_obj(struct fsl_mc_io *mc_io, > obj_desc->ver_major = le16_to_cpu(rsp_params->version_major); > obj_desc->ver_minor = le16_to_cpu(rsp_params->version_minor); > obj_desc->flags = le16_to_cpu(rsp_params->flags); > - strncpy(obj_desc->type, rsp_params->type, 16); > - obj_desc->type[15] = '\0'; > - strncpy(obj_desc->label, rsp_params->label, 16); > - obj_desc->label[15] = '\0'; > + strscpy_pad(obj_desc->type, rsp_params->type, 16); > + strscpy_pad(obj_desc->label, rsp_params->label, 16); I really don't like all the open-coded sizes, but yeah, okay, it's not technically wrong. :) Reviewed-by: Kees Cook -Kees > return 0; > } > EXPORT_SYMBOL_GPL(dprc_get_obj); > @@ -491,8 +489,7 @@ int dprc_set_obj_irq(struct fsl_mc_io *mc_io, > cmd_params->irq_addr = cpu_to_le64(irq_cfg->paddr); > cmd_params->irq_num = cpu_to_le32(irq_cfg->irq_num); > cmd_params->obj_id = cpu_to_le32(obj_id); > - strncpy(cmd_params->obj_type, obj_type, 16); > - cmd_params->obj_type[15] = '\0'; > + strscpy_pad(cmd_params->obj_type, obj_type, 16); > > /* send command to mc*/ > return mc_send_command(mc_io, &cmd); > @@ -564,8 +561,7 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io, > cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params; > cmd_params->obj_id = cpu_to_le32(obj_id); > cmd_params->region_index = region_index; > - strncpy(cmd_params->obj_type, obj_type, 16); > - cmd_params->obj_type[15] = '\0'; > + strscpy_pad(cmd_params->obj_type, obj_type, 16); > > /* send command to mc*/ > err = mc_send_command(mc_io, &cmd); > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230912-strncpy-drivers-bus-fsl-mc-dprc-c-bc7d818386ec > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH][next] checkpatch: add a couple new alloc functions to alloc with multiplies check
ltiply_apis) {
> + $alloc_with_multiply_search .= '|' if ($alloc_with_multiply_search ne
> "");
> + $alloc_with_multiply_search .= $entry;
> +}
> +$alloc_with_multiply_search = "(?:${alloc_with_multiply_search})";
> +
> our $mode_perms_world_writable = qr{
> S_IWUGO |
> S_IWOTH |
> @@ -7210,14 +7227,11 @@ sub process {
> # check for (kv|k)[mz]alloc with multiplies that could be
> kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> if ($perl_version_ok &&
> defined $stat &&
> - $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> {
> + $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> {
> my $oldfunc = $3;
> + my $newfunc = $alloc_with_multiply_apis{$oldfunc};
> my $a1 = $4;
> my $a2 = $10;
> - my $newfunc = "kmalloc_array";
> - $newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> - $newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
> - $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> my $r1 = $a1;
> my $r2 = $a2;
> if ($a1 =~ /^sizeof\s*\S/) {
> @@ -7233,7 +7247,7 @@ sub process {
>"Prefer $newfunc over $oldfunc with
> multiply\n" . $herectx) &&
> $cnt == 1 &&
> $fix) {
> - $fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> + $fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> }
> }
> }
>
--
Kees Cook
Re: [PATCH v2][next] RDMA/core: Use size_{add,mul}() in calls to struct_size()
On Mon, Sep 11, 2023 at 05:27:59PM -0600, Gustavo A. R. Silva wrote:
> Harden calls to struct_size() with size_add() and size_mul().
Specifically, make sure that open-coded arithmetic cannot cause an
overflow/wraparound. (i.e. it will stay saturated at SIZE_MAX.)
>
> Fixes: 467f432a521a ("RDMA/core: Split port and device counter sysfs
> attributes")
> Fixes: a4676388e2e2 ("RDMA/core: Simplify how the gid_attrs sysfs is created")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
-Kees
> ---
> Changes in v2:
> - Update changelog text: remove the part about binary differences (it
>was added by mistake).
>
> drivers/infiniband/core/sysfs.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index ee59d7391568..ec5efdc16660 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -903,7 +903,7 @@ alloc_hw_stats_device(struct ib_device *ibdev)
>* Two extra attribue elements here, one for the lifespan entry and
>* one to NULL terminate the list for the sysfs core code
>*/
> - data = kzalloc(struct_size(data, attrs, stats->num_counters + 1),
> + data = kzalloc(struct_size(data, attrs, size_add(stats->num_counters,
> 1)),
> GFP_KERNEL);
> if (!data)
> goto err_free_stats;
> @@ -1009,7 +1009,7 @@ alloc_hw_stats_port(struct ib_port *port, struct
> attribute_group *group)
>* Two extra attribue elements here, one for the lifespan entry and
>* one to NULL terminate the list for the sysfs core code
>*/
> - data = kzalloc(struct_size(data, attrs, stats->num_counters + 1),
> + data = kzalloc(struct_size(data, attrs, size_add(stats->num_counters,
> 1)),
> GFP_KERNEL);
> if (!data)
> goto err_free_stats;
> @@ -1140,7 +1140,7 @@ static int setup_gid_attrs(struct ib_port *port,
> int ret;
>
> gid_attr_group = kzalloc(struct_size(gid_attr_group, attrs_list,
> - attr->gid_tbl_len * 2),
> + size_mul(attr->gid_tbl_len, 2)),
>GFP_KERNEL);
> if (!gid_attr_group)
> return -ENOMEM;
> @@ -1205,8 +1205,8 @@ static struct ib_port *setup_port(struct ib_core_device
> *coredev, int port_num,
> int ret;
>
> p = kvzalloc(struct_size(p, attrs_list,
> - attr->gid_tbl_len + attr->pkey_tbl_len),
> - GFP_KERNEL);
> + size_add(attr->gid_tbl_len,
> attr->pkey_tbl_len)),
> + GFP_KERNEL);
> if (!p)
> return ERR_PTR(-ENOMEM);
> p->ibdev = device;
> --
> 2.34.1
>
--
Kees Cook
Re: [PATCH] auxdisplay: panel: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 08:51:04PM +, Justin Stitt wrote:
> `strncpy` is deprecated and as such we should prefer more robust and
> less ambiguous interfaces.
>
> In this case, all of `press_str`, `repeat_str` and `release_str` are
> explicitly marked as nonstring:
> | struct { /* valid when type == INPUT_TYPE_KBD */
> | char press_str[sizeof(void *) + sizeof(int)] __nonstring;
> | char repeat_str[sizeof(void *) + sizeof(int)] __nonstring;
> | char release_str[sizeof(void *) + sizeof(int)] __nonstring;
> | } kbd;
>
> ... which makes `strtomem_pad` a suitable replacement as it is
> functionally the same whilst being more obvious about its behavior.
Yup, this is exactly what strtomem_pad() was made for. :)
Reviewed-by: Kees Cook
-Kees
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested
> ---
> drivers/auxdisplay/panel.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
> index eba04c0de7eb..e20d35bdf5fe 100644
> --- a/drivers/auxdisplay/panel.c
> +++ b/drivers/auxdisplay/panel.c
> @@ -1449,10 +1449,9 @@ static struct logical_input *panel_bind_key(const char
> *name, const char *press,
> key->rise_time = 1;
> key->fall_time = 1;
>
> - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
> - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
> - strncpy(key->u.kbd.release_str, release,
> - sizeof(key->u.kbd.release_str));
> + strtomem_pad(key->u.kbd.press_str, press, '\0');
> + strtomem_pad(key->u.kbd.repeat_str, repeat, '\0');
> + strtomem_pad(key->u.kbd.release_str, release, '\0');
> list_add(&key->list, &logical_inputs);
> return key;
> }
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230911-strncpy-drivers-auxdisplay-panel-c-83bce51f32cb
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] ACPI: OSI: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 08:36:44PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We know `osi->string` is a NUL-terminated string due to its eventual use
> in `acpi_install_interface()` and `acpi_remove_interface()` which expect
> a `acpi_string` which has been specifically typedef'd as:
> | typedef char *acpi_string; /* Null terminated ASCII string */
>
> ... and which also has other string functions used on it like `strlen`.
> Furthermore, padding is not needed in this instance either.
Following the callers, I agree, this doesn't need %NUL padding -- it's
always processed as a regular C string.
Reviewed-by: Kees Cook
-Kees
>
> Due to the reasoning above a suitable replacement is `strscpy` [2] since
> it guarantees NUL-termination on the destination buffer and doesn't
> unnecessarily NUL-pad.
>
> While there is unlikely to be a buffer overread (or other related bug)
> in this case, we should still favor a more robust and less ambiguous
> interface.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested
> ---
> drivers/acpi/osi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
> index d4405e1ca9b9..df9328c850bd 100644
> --- a/drivers/acpi/osi.c
> +++ b/drivers/acpi/osi.c
> @@ -110,7 +110,7 @@ void __init acpi_osi_setup(char *str)
> break;
> } else if (osi->string[0] == '\0') {
> osi->enable = enable;
> - strncpy(osi->string, str, OSI_STRING_LENGTH_MAX);
> + strscpy(osi->string, str, OSI_STRING_LENGTH_MAX);
> break;
> }
> }
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230911-strncpy-drivers-acpi-osi-c-c801b7427987
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] xen/efi: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 06:59:31PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > `efi_loader_signature` has space for 4 bytes. We are copying "Xen" (3 bytes) > plus a NUL-byte which makes 4 total bytes. With that being said, there is > currently not a bug with the current `strncpy()` implementation in terms of > buffer overreads but we should favor a more robust string interface > either way. Yeah, this will work. Since this is a u32 destination, I do wonder if strtomem_pad() would be better since we're not really writing a string? But since this is all hard-coded, it doesn't matter. :) Reviewed-by: Kees Cook -Kees > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer while being functionally the > same in this case. > > Link: > www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Cc: Kees Cook > Signed-off-by: Justin Stitt > --- > Note: build-tested > --- > arch/x86/xen/efi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c > index 863d0d6b3edc..7250d0e0e1a9 100644 > --- a/arch/x86/xen/efi.c > +++ b/arch/x86/xen/efi.c > @@ -138,7 +138,7 @@ void __init xen_efi_init(struct boot_params *boot_params) > if (efi_systab_xen == NULL) > return; > > - strncpy((char *)&boot_params->efi_info.efi_loader_signature, "Xen", > + strscpy((char *)&boot_params->efi_info.efi_loader_signature, "Xen", > sizeof(boot_params->efi_info.efi_loader_signature)); > boot_params->efi_info.efi_systab = (__u32)__pa(efi_systab_xen); > boot_params->efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> > 32); > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230911-strncpy-arch-x86-xen-efi-c-14292f5a79ee > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH] x86/tdx: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 03:01:16PM -0700, Justin Stitt wrote: > On Mon, Sep 11, 2023 at 11:51 AM Dave Hansen wrote: > > > > On 9/11/23 11:27, Justin Stitt wrote: > > > `strncpy` is deprecated and we should prefer more robust string apis. > > > > I dunno. It actually seems like a pretty good fit here. > > > > > In this case, `message.str` is not expected to be NUL-terminated as it > > > is simply a buffer of characters residing in a union which allows for > > > named fields representing 8 bytes each. There is only one caller of > > > `tdx_panic()` and they use a 59-length string for `msg`: > > > | const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute > > > must be set."; > > > > I'm not really following this logic. > > > > We need to do the following: > > > > 1. Make sure not to over write past the end of 'message' > > 2. Make sure not to over read past the end of 'msg' > > 3. Make sure not to leak stack data into the hypercall registers > >in the case of short strings. > > > > strncpy() does #1, #2 and #3 just fine. > > Right, to be clear, I do not suspect a bug in the current > implementation. Rather, let's move towards a less ambiguous interface > for maintainability's sake > > > > > The resulting string does *NOT* need to be NULL-terminated. See the > > comment: > > > > /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */ > > > > Are there cases where strncpy() doesn't NULL-terminate the string other > > than when the buffer is full? > > > > I actually didn't realize that strncpy() pads its output up to the full > > size. I wonder if Kirill used it intentionally or whether he got lucky > > here. :) > > Big reason to use strtomem_pad as it is more obvious about what it does. > > I'd love more thoughts/testing here. This looks like exactly the right conversion: strtomem_pad() will do 1, 2, and 3 (and does it unambiguously and without allowing for a possible-wrong "size" parameter for the destination buffer). Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] um,ethertap: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 01:12:35PM -0700, Justin Stitt wrote: > On Tue, Sep 12, 2023 at 12:36 AM Geert Uytterhoeven > wrote: > > > > Hi Justin, > > > > Thanks for your patch! > > > > On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > > > `gate_buf` should always be NUL-terminated and does not require > > > NUL-padding. It is used as a string arg inside an argv array given to > > > > Can you please explain why it does not require NUL-padding? > > It looks like this buffer is passed eventually to a user space > > application, thus possibly leaking uninitialized stack data. > > It looks like it's being passed as a list of command-line arguments in > `run_helper()`. Should this be NUL-padded due to its eventual use in > user space? If we think yes I can send a v2. Thanks for pointing this > out. No, it's passed as a pointer to a string, and the clone call will ultimately make a copy-until-%NUL when building the new process. This doesn't need padding. Reviewed-by: Kees Cook -Kees -- Kees Cook
Re: [PATCH] vt: Fix potential read overflow of kernel memory
On Thu, Aug 31, 2023 at 10:23:10AM -0400, Azeem Shaikh wrote: > Are folks ok with me sending out a v2 for this with a better commit > log that explains the issue? Yes, please do. It should clear up the questions from this thread. :) Thanks! -Kees -- Kees Cook
Re: linux-next: Tree for Sep 12 (bcachefs)
On Thu, Sep 14, 2023 at 03:38:07PM -0400, Kent Overstreet wrote:
> On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> > It looks like you just want a type union for the flexible array.
> > This can be done like this:
> >
> > struct bch_sb_field_journal_seq_blacklist {
> > struct bch_sb_field field;
> >
> > union {
> > DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> > DECLARE_FLEX_ARRAY(__u64, _data);
> > };
> > };
>
> Eesh, why though?
>
> Honestly, I'm not a fan of the change to get rid of zero size arrays,
> this seems to be adding a whole lot of macro layering and indirection
> for nothing.
The C standard doesn't help us in that regard, that's true. But we've
been working to get it fixed. For example, there's discussion happening
next week at GNU Cauldron about flexible arrays in unions. It's already
possible, so better to just fix the standard -- real world code needs it
and uses it, as the bcachefs code illustrates. :)
> The only thing a zero size array could possibly be is a flexible array
> member or a marker, why couldn't we have just kept treating zero size
> arrays like flexible array members?
Because they're ambiguous and then the compiler can't do appropriate
bounds checking, compile-time diagnostics, etc. Maybe it's actually zero
sized, maybe it's not. Nothing stops them from being in the middle of
the structure so if someone accidentally tries to put members after it
(which has happened before), we end up with bizarre corruptions, etc,
etc. Flexible arrays are unambiguous, and that's why we committed to
converting all the fake flex arrays. The compiler does not have to guess
(or as has been the case: give up on) figuring out what was intended.
Regardless, I'm just trying to help make sure folks that run with
CONFIG_UBSAN_BOUNDS=y (as done in Android, Ubuntu, etc) will be able to
use bcachefs without runtime warnings, etc. Indexing through a 0-sized
array is going to trip the diagnostic either at runtime or when building
with -Warray-bounds.
-Kees
--
Kees Cook
Re: linux-next: Tree for Sep 12 (bcachefs)
On Tue, Sep 12, 2023 at 03:26:45PM +1000, Stephen Rothwell wrote:
> New tree: bcachefs
Thanks for going through and fixing all the fake flexible array members.
It looks much nicer. :)
I have some questions about the remaining "markers", for example:
$ git grep -A8 '\bkey_start\b' -- fs/bcachefs
fs/bcachefs/bcachefs_format.h: __u8key_start[0];
...
fs/bcachefs/bcachefs_format.h- __u8pad[sizeof(struct bkey) - 3];
--
fs/bcachefs/bkey.c: u8 *l = k->key_start;
Why isn't this just:
u8 *l = k->pad
and you can drop the marker?
And some seem entirely unused, like all of "struct bch_reflink_v".
And some are going to fail at runtime, since they're still zero-sized
and being used as an actual array:
struct bch_sb_field_journal_seq_blacklist {
struct bch_sb_field field;
struct journal_seq_blacklist_entry start[0];
__u64 _data[];
};
...
memmove(&bl->start[i],
&bl->start[i + 1],
sizeof(bl->start[0]) * (nr - i));
It looks like you just want a type union for the flexible array.
This can be done like this:
struct bch_sb_field_journal_seq_blacklist {
struct bch_sb_field field;
union {
DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
DECLARE_FLEX_ARRAY(__u64, _data);
};
};
Hopefully that helps!
-Kees
--
Kees Cook
[PATCH v2] module: Clarify documentation of module_param_call()
Commit 9bbb9e5a3310 ("param: use ops in struct kernel_param, rather than
get and set fns directly") added the comment that module_param_call()
was deprecated, during a large scale refactoring to bring sanity to type
casting back then. In 2017 following more cleanups, it became useful
again as it wraps a common pattern of creating an ops struct for a
given get/set pair:
b2f270e87473 ("module: Prepare to convert all module_param_call() prototypes")
ece1996a21ee ("module: Do not paper over type mismatches in
module_param_call()")
static const struct kernel_param_ops __param_ops_##name = \
{ .flags = 0, .set = _set, .get = _get }; \
__module_param_call(MODULE_PARAM_PREFIX, \
name, &__param_ops_##name, arg, perm, -1, 0)
__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0)
Many users of module_param_cb() appear to be almost universally
open-coding the same thing that module_param_call() does now. Don't
discourage[1] people from using module_param_call(): clarify the comment
to show that module_param_cb() is useful if you repeatedly use the same
pair of get/set functions.
[1] https://lore.kernel.org/lkml/202308301546.5C789E5EC@keescook/
Cc: Luis Chamberlain
Cc: Johan Hovold
Cc: Jessica Yu
Cc: Sagi Grimberg
Cc: Nick Desaulniers
Cc: Miguel Ojeda
Cc: Joe Perches
Cc: linux-modu...@vger.kernel.org
Reviewed-by: Miguel Ojeda
Signed-off-by: Kees Cook
---
Luis, I note that include/linux/moduleparam.h isn't in the MAINTAINERS
file pattern. Perhaps you want to use include/linux/module*.h?
---
include/linux/moduleparam.h | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 962cd41a2cb5..d4452f93d060 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -293,7 +293,11 @@ struct kparam_array
= { __param_str_##name, THIS_MODULE, ops, \
VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } }
-/* Obsolete - use module_param_cb() */
+/*
+ * Useful for describing a set/get pair used only once (i.e. for this
+ * parameter). For repeated set/get pairs (i.e. the same struct
+ * kernel_param_ops), use module_param_cb() instead.
+ */
#define module_param_call(name, _set, _get, arg, perm) \
static const struct kernel_param_ops __param_ops_##name = \
{ .flags = 0, .set = _set, .get = _get }; \
--
2.34.1
Re: [PATCH 12/19] fs: convert kill_litter_super to litter_shutdown_sb
On Wed, Sep 13, 2023 at 08:10:06AM -0300, Christoph Hellwig wrote: > Replace kill_litter_super with litter_shutdown_sb, which is wired up to > the ->shutdown_sb method. For file systems that wrapped > kill_litter_super, ->kill_sb is replaced with ->shutdown and ->free_sb > methods as needed. > > Signed-off-by: Christoph Hellwig Reviewed-by: Kees Cook # for pstore -- Kees Cook
Re: [PATCH 08/19] pstore: shrink the pstore_sb_lock critical section in pstore_kill_sb
On Wed, Sep 13, 2023 at 08:10:02AM -0300, Christoph Hellwig wrote: > ->kill_sb can't race with creating ->fill_super because pstore is a > _single file system that only ever has a single sb instance, and we wait > for the previous one to go away before creating a new one. Reduce > the critical section so that is is not held over generic_shutdown_super. > > Signed-off-by: Christoph Hellwig Thanks for the refactoring! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v5] Randomized slab caches for kmalloc()
g like > [`CONFIG_KMALLOC_SPLIT_VARSIZE`](https://github.com/thejh/linux/blob/slub-virtual/MITIGATION_README) > wouldn't significantly increase complexity. Now that we have a mechanism to easily deal with "many kmalloc buckets", I think we can easily start carving out specific variable-sized caches (like msg_msg). Basically doing a manual type-based separation. So, yeah, we're in a better place that we were before, and better positioned to continue to make improvements here. I think an easy win would be doing this last one: separate out the user controlled variable-sized caches and give them their own distinct buckets outside of the 16 random ones. Can you give that a try and send patches? -Kees -- Kees Cook
Re: [PATCH] arm64: Show three registers per line
On Tue, Apr 20, 2021 at 06:22:45PM +0100, Matthew Wilcox (Oracle) wrote: > Displaying two registers per line takes 15 lines. That improves to just > 10 lines if we display three registers per line, which reduces the amount > of information lost when oopses are cut off. It stays within 80 columns > and matches x86-64. > > Signed-off-by: Matthew Wilcox (Oracle) Reviewed-by: Kees Cook -- Kees Cook
