[Xen-devel] [PATCH 1/3] treewide: Lift switch variables out of switches
Variables declared in a switch statement before any case statements
cannot be initialized, so move all instances out of the switches.
After this, future always-initialized stack variables will work
and not throw warnings like this:
fs/fcntl.c: In function ‘send_sigio_to_task’:
fs/fcntl.c:738:13: warning: statement will never be executed
[-Wswitch-unreachable]
siginfo_t si;
^~
Signed-off-by: Kees Cook
---
arch/x86/xen/enlighten_pv.c | 7 ---
drivers/char/pcmcia/cm4000_cs.c | 2 +-
drivers/char/ppdev.c | 20 ---
drivers/gpu/drm/drm_edid.c| 4 ++--
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 4 ++--
drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ++-
drivers/tty/n_tty.c | 3 +--
drivers/usb/gadget/udc/net2280.c | 5 ++---
fs/fcntl.c| 3 ++-
mm/shmem.c| 5 +++--
net/core/skbuff.c | 4 ++--
net/ipv6/ip6_gre.c| 4 ++--
net/ipv6/ip6_tunnel.c | 4 ++--
net/openvswitch/flow_netlink.c| 7 +++
security/tomoyo/common.c | 3 ++-
security/tomoyo/condition.c | 7 ---
security/tomoyo/util.c| 4 ++--
18 files changed, 45 insertions(+), 46 deletions(-)
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c54a493e139a..a79d4b548a08 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -907,14 +907,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
{
int ret;
+#ifdef CONFIG_X86_64
+ unsigned which;
+ u64 base;
+#endif
ret = 0;
switch (msr) {
#ifdef CONFIG_X86_64
- unsigned which;
- u64 base;
-
case MSR_FS_BASE: which = SEGBASE_FS; goto set;
case MSR_KERNEL_GS_BASE:which = SEGBASE_GS_USER; goto set;
case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set;
diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 7a4eb86aedac..7211dc0e6f4f 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -663,6 +663,7 @@ static void monitor_card(struct timer_list *t)
{
struct cm4000_dev *dev = from_timer(dev, t, timer);
unsigned int iobase = dev->p_dev->resource[0]->start;
+ unsigned char flags0;
unsigned short s;
struct ptsreq ptsreq;
int i, atrc;
@@ -731,7 +732,6 @@ static void monitor_card(struct timer_list *t)
}
switch (dev->mstate) {
- unsigned char flags0;
case M_CARDOFF:
DEBUGP(4, dev, "M_CARDOFF\n");
flags0 = inb(REG_FLAGS0(iobase));
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 1ae77b41050a..d77c97e4f996 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -359,14 +359,19 @@ static int pp_do_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
struct pp_struct *pp = file->private_data;
struct parport *port;
void __user *argp = (void __user *)arg;
+ struct ieee1284_info *info;
+ unsigned char reg;
+ unsigned char mask;
+ int mode;
+ s32 time32[2];
+ s64 time64[2];
+ struct timespec64 ts;
+ int ret;
/* First handle the cases that don't take arguments. */
switch (cmd) {
case PPCLAIM:
{
- struct ieee1284_info *info;
- int ret;
-
if (pp->flags & PP_CLAIMED) {
dev_dbg(&pp->pdev->dev, "you've already got it!\n");
return -EINVAL;
@@ -517,15 +522,6 @@ static int pp_do_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
port = pp->pdev->port;
switch (cmd) {
- struct ieee1284_info *info;
- unsigned char reg;
- unsigned char mask;
- int mode;
- s32 time32[2];
- s64 time64[2];
- struct timespec64 ts;
- int ret;
-
case PPRSTATUS:
reg = parport_read_status(port);
if (copy_to_user(argp, ®, sizeof(reg)))
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b506e3622b08..8f93956c1628 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3942,12 +3942,12 @@ static void drm_edid_to_eld(struct drm_connector
*connector, struct edid *edid)
}
for_each_cea_db(cea, i, start, end) {
+
[Xen-devel] [PATCH 3/3] lib: Introduce test_stackinit module
Adds test for stack initialization coverage. We have several build options that control the level of stack variable initialization. This test lets us visualize which options cover which cases, and provide tests for options that are currently not available (padding initialization). All options pass the explicit initialization cases and the partial initializers (even with padding): test_stackinit: u8_zero ok test_stackinit: u16_zero ok test_stackinit: u32_zero ok test_stackinit: u64_zero ok test_stackinit: char_array_zero ok test_stackinit: small_hole_zero ok test_stackinit: big_hole_zero ok test_stackinit: packed_zero ok test_stackinit: small_hole_dynamic_partial ok test_stackinit: big_hole_dynamic_partial ok test_stackinit: packed_static_partial ok test_stackinit: small_hole_static_partial ok test_stackinit: big_hole_static_partial ok The results of the other tests (which contain no explicit initialization), change based on the build's configured compiler instrumentation. No options: test_stackinit: small_hole_static_all FAIL (uninit bytes: 3) test_stackinit: big_hole_static_all FAIL (uninit bytes: 61) test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3) test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61) test_stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23) test_stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127) test_stackinit: small_hole_runtime_all FAIL (uninit bytes: 3) test_stackinit: big_hole_runtime_all FAIL (uninit bytes: 61) test_stackinit: u8 FAIL (uninit bytes: 1) test_stackinit: u16 FAIL (uninit bytes: 2) test_stackinit: u32 FAIL (uninit bytes: 4) test_stackinit: u64 FAIL (uninit bytes: 8) test_stackinit: char_array FAIL (uninit bytes: 16) test_stackinit: small_hole FAIL (uninit bytes: 24) test_stackinit: big_hole FAIL (uninit bytes: 128) test_stackinit: user FAIL (uninit bytes: 32) test_stackinit: failures: 16 CONFIG_GCC_PLUGIN_STRUCTLEAK=y This only tries to initialize structs with __user markings: test_stackinit: small_hole_static_all FAIL (uninit bytes: 3) test_stackinit: big_hole_static_all FAIL (uninit bytes: 61) test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3) test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61) test_stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23) test_stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127) test_stackinit: small_hole_runtime_all FAIL (uninit bytes: 3) test_stackinit: big_hole_runtime_all FAIL (uninit bytes: 61) test_stackinit: u8 FAIL (uninit bytes: 1) test_stackinit: u16 FAIL (uninit bytes: 2) test_stackinit: u32 FAIL (uninit bytes: 4) test_stackinit: u64 FAIL (uninit bytes: 8) test_stackinit: char_array FAIL (uninit bytes: 16) test_stackinit: small_hole FAIL (uninit bytes: 24) test_stackinit: big_hole FAIL (uninit bytes: 128) test_stackinit: user ok test_stackinit: failures: 15 CONFIG_GCC_PLUGIN_STRUCTLEAK=y CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y This initializes all structures passed by reference (scalars and strings remain uninitialized, but padding is wiped): test_stackinit: small_hole_static_all ok test_stackinit: big_hole_static_all ok test_stackinit: small_hole_dynamic_all ok test_stackinit: big_hole_dynamic_all ok test_stackinit: small_hole_runtime_partial ok test_stackinit: big_hole_runtime_partial ok test_stackinit: small_hole_runtime_all ok test_stackinit: big_hole_runtime_all ok test_stackinit: u8 FAIL (uninit bytes: 1) test_stackinit: u16 FAIL (uninit bytes: 2) test_stackinit: u32 FAIL (uninit bytes: 4) test_stackinit: u64 FAIL (uninit bytes: 8) test_stackinit: char_array FAIL (uninit bytes: 16) test_stackinit: small_hole ok test_stackinit: big_hole ok test_stackinit: user ok test_stackinit: failures: 5 CONFIG_GCC_PLUGIN_STACKINIT=y This initializes all variables, but has no special padding handling: test_stackinit: small_hole_static_all FAIL (uninit bytes: 3) test_stackinit: big_hole_static_all FAIL (uninit bytes: 61) test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3) test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61) test_stackinit: small_hole_runtime_partial ok test_stackinit: big_hole_runtime_partial ok test_stackinit: small_hole_runtime_all ok test_stackinit: big_hole_runtime_all ok test_stackinit: u8 ok test_stackinit: u16 ok test_stackinit: u32 ok test_stackinit: u64 ok test_stackinit: char_array ok test_stackinit: small_hole ok test_stackinit: big_hole ok test_stackinit: user ok test_stackinit: failures: 4 Signed-off-by: Kees Cook --- lib/Kconfig.debug| 9 ++ lib/Makefile | 1 + lib/test_stackinit.c | 327 +++ 3 files changed, 337 insertions(+) create mode 100644 lib/test_stackinit.c diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index d4df5b24d75e..09788af9 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2001,6 +2001,15 @@ config TEST_OBJAGG If unsure, say N. +config TEST_STACKINIT + tristate "Test leve
[Xen-devel] [PATCH 0/3] gcc-plugins: Introduce stackinit plugin
This adds a new plugin "stackinit" that attempts to perform unconditional initialization of all stack variables[1]. It has wider effects than GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y since BYREF_ALL does not consider non-structures. A notable weakness is that padding bytes in many cases remain uninitialized since GCC treats these bytes as "undefined". I'm hoping we can improve the compiler (or the plugin) to cover that too. (It's worth noting that BYREF_ALL actually does handle the padding -- I think this is due to the different method of detecting if initialization is needed.) Included is a tree-wide change to move switch variables up and out of their switch and into the top-level variable declarations. Included is a set of test cases for evaluating stack initialization, which checks for padding, different types, etc. Feedback welcome! :) -Kees [1] https://lkml.kernel.org/r/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j94...@mail.gmail.com Kees Cook (3): treewide: Lift switch variables out of switches gcc-plugins: Introduce stackinit plugin lib: Introduce test_stackinit module arch/x86/xen/enlighten_pv.c | 7 +- drivers/char/pcmcia/cm4000_cs.c | 2 +- drivers/char/ppdev.c | 20 +- drivers/gpu/drm/drm_edid.c| 4 +- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_pm.c | 4 +- drivers/net/ethernet/intel/e1000/e1000_main.c | 3 +- drivers/tty/n_tty.c | 3 +- drivers/usb/gadget/udc/net2280.c | 5 +- fs/fcntl.c| 3 +- lib/Kconfig.debug | 9 + lib/Makefile | 1 + lib/test_stackinit.c | 327 ++ mm/shmem.c| 5 +- net/core/skbuff.c | 4 +- net/ipv6/ip6_gre.c| 4 +- net/ipv6/ip6_tunnel.c | 4 +- net/openvswitch/flow_netlink.c| 7 +- scripts/Makefile.gcc-plugins | 6 + scripts/gcc-plugins/Kconfig | 9 + scripts/gcc-plugins/gcc-common.h | 11 +- scripts/gcc-plugins/stackinit_plugin.c| 79 + security/tomoyo/common.c | 3 +- security/tomoyo/condition.c | 7 +- security/tomoyo/util.c| 4 +- 25 files changed, 484 insertions(+), 49 deletions(-) create mode 100644 lib/test_stackinit.c create mode 100644 scripts/gcc-plugins/stackinit_plugin.c -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/3] gcc-plugins: Introduce stackinit plugin
This attempts to duplicate the proposed gcc option -finit-local-vars[1]
in an effort to implement the "always initialize local variables" kernel
development goal[2].
Enabling CONFIG_GCC_PLUGIN_STACKINIT should stop all "uninitialized
stack variable" flaws as long as they don't depend on being zero. :)
[1] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html
[2]
https://lkml.kernel.org/r/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j94...@mail.gmail.com
Signed-off-by: Kees Cook
---
scripts/Makefile.gcc-plugins | 6 ++
scripts/gcc-plugins/Kconfig| 9 +++
scripts/gcc-plugins/gcc-common.h | 11 +++-
scripts/gcc-plugins/stackinit_plugin.c | 79 ++
4 files changed, 102 insertions(+), 3 deletions(-)
create mode 100644 scripts/gcc-plugins/stackinit_plugin.c
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index 35042d96cf5d..2483121d781c 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -12,6 +12,12 @@ export DISABLE_LATENT_ENTROPY_PLUGIN
gcc-plugin-$(CONFIG_GCC_PLUGIN_SANCOV) += sancov_plugin.so
+gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKINIT) += stackinit_plugin.so
+ifdef CONFIG_GCC_PLUGIN_STACKINIT
+DISABLE_STACKINIT_PLUGIN += -fplugin-arg-stackinit_plugin-disable
+endif
+export DISABLE_STACKINIT_PLUGIN
+
gcc-plugin-$(CONFIG_GCC_PLUGIN_STRUCTLEAK) += structleak_plugin.so
gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE) \
+= -fplugin-arg-structleak_plugin-verbose
diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index d45f7f36b859..b117fe83f1d3 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -66,6 +66,15 @@ config GCC_PLUGIN_LATENT_ENTROPY
* https://grsecurity.net/
* https://pax.grsecurity.net/
+config GCC_PLUGIN_STACKINIT
+ bool "Initialize all stack variables to zero by default"
+ depends on GCC_PLUGINS
+ depends on !GCC_PLUGIN_STRUCTLEAK
+ help
+ This plugin zero-initializes all stack variables. This is more
+ comprehensive than GCC_PLUGIN_STRUCTLEAK, and attempts to
+ duplicate the proposed -finit-local-vars gcc build flag.
+
config GCC_PLUGIN_STRUCTLEAK
bool "Force initialization of variables containing userspace addresses"
# Currently STRUCTLEAK inserts initialization out of live scope of
diff --git a/scripts/gcc-plugins/gcc-common.h b/scripts/gcc-plugins/gcc-common.h
index 552d5efd7cb7..f690b4deeabd 100644
--- a/scripts/gcc-plugins/gcc-common.h
+++ b/scripts/gcc-plugins/gcc-common.h
@@ -76,6 +76,14 @@
#include "c-common.h"
#endif
+#if BUILDING_GCC_VERSION > 4005
+#include "c-tree.h"
+#else
+/* should come from c-tree.h if only it were installed for gcc 4.5... */
+#define C_TYPE_FIELDS_READONLY(TYPE) TREE_LANG_FLAG_1(TYPE)
+extern bool global_bindings_p (void);
+#endif
+
#if BUILDING_GCC_VERSION <= 4008
#include "tree-flow.h"
#else
@@ -158,9 +166,6 @@ void dump_gimple_stmt(pretty_printer *, gimple, int, int);
#define TYPE_NAME_POINTER(node) IDENTIFIER_POINTER(TYPE_NAME(node))
#define TYPE_NAME_LENGTH(node) IDENTIFIER_LENGTH(TYPE_NAME(node))
-/* should come from c-tree.h if only it were installed for gcc 4.5... */
-#define C_TYPE_FIELDS_READONLY(TYPE) TREE_LANG_FLAG_1(TYPE)
-
static inline tree build_const_char_string(int len, const char *str)
{
tree cstr, elem, index, type;
diff --git a/scripts/gcc-plugins/stackinit_plugin.c
b/scripts/gcc-plugins/stackinit_plugin.c
new file mode 100644
index ..41055cd7098e
--- /dev/null
+++ b/scripts/gcc-plugins/stackinit_plugin.c
@@ -0,0 +1,79 @@
+/* SPDX-License: GPLv2 */
+/*
+ * This will zero-initialize local stack variables. (Though structure
+ * padding may remain uninitialized in certain cases.)
+ *
+ * Implements Florian Weimer's "-finit-local-vars" gcc patch as a plugin:
+ * https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html
+ *
+ * Plugin skeleton code thanks to PaX Team.
+ *
+ * Options:
+ * -fplugin-arg-stackinit_plugin-disable
+ */
+
+#include "gcc-common.h"
+
+__visible int plugin_is_GPL_compatible;
+
+static struct plugin_info stackinit_plugin_info = {
+ .version= "20190122",
+ .help = "disable\tdo not activate plugin\n",
+};
+
+static void finish_decl(void *event_data, void *data)
+{
+ tree decl = (tree)event_data;
+ tree type;
+
+ if (TREE_CODE (decl) != VAR_DECL)
+ return;
+
+ if (DECL_EXTERNAL (decl))
+ return;
+
+ if (DECL_INITIAL (decl) != NULL_TREE)
+ return;
+
+ if (global_bindings_p ())
+ return;
+
+ type = TREE_TYPE (decl);
+ if (AGGREGATE_TYPE_P (type))
+ DECL_INITIAL (decl) = build_constructor (type, NULL);
+ else
+
Re: [Xen-devel] [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
On Thu, Jan 24, 2019 at 4:44 AM Jani Nikula wrote:
>
> On Wed, 23 Jan 2019, Edwin Zimmerman wrote:
> > On Wed, 23 Jan 2019, Jani Nikula wrote:
> >> On Wed, 23 Jan 2019, Greg KH wrote:
> >> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> >> >> Variables declared in a switch statement before any case statements
> >> >> cannot be initialized, so move all instances out of the switches.
> >> >> After this, future always-initialized stack variables will work
> >> >> and not throw warnings like this:
> >> >>
> >> >> fs/fcntl.c: In function ‘send_sigio_to_task’:
> >> >> fs/fcntl.c:738:13: warning: statement will never be executed
> >> >> [-Wswitch-unreachable]
> >> >>siginfo_t si;
> >> >> ^~
> >> >
> >> > That's a pain, so this means we can't have any new variables in { }
> >> > scope except for at the top of a function?
Just in case this wasn't clear: no, it's just the switch statement
before the first "case". I cannot imagine how bad it would be if we
couldn't have block-scoped variables! Heh. :)
> >> >
> >> > That's going to be a hard thing to keep from happening over time, as
> >> > this is valid C :(
> >>
> >> Not all valid C is meant to be used! ;)
> >
> > Very true. The other thing to keep in mind is the burden of enforcing
> > a prohibition on a valid C construct like this. It seems to me that
> > patch reviewers and maintainers have enough to do without forcing them
> > to watch for variable declarations in switch statements. Automating
> > this prohibition, should it be accepted, seems like a good idea to me.
>
> Considering that the treewide diffstat to fix this is:
>
> 18 files changed, 45 insertions(+), 46 deletions(-)
>
> and using the gcc plugin in question will trigger the switch-unreachable
> warning, I think we're good. There'll probably be the occasional
> declarations that pass through, and will get fixed afterwards.
Yeah, that was my thinking as well: it's a rare use, and we get a
warning when it comes up.
Thanks!
--
Kees Cook
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
On Thu, Jan 24, 2019 at 8:18 AM Matthew Wilcox wrote:
>
> On Wed, Jan 23, 2019 at 04:17:30PM +0200, Jani Nikula wrote:
> > Can't have:
> >
> > switch (i) {
> > int j;
> > case 0:
> > /* ... */
> > }
> >
> > because it can't be turned into:
> >
> > switch (i) {
> > int j = 0; /* not valid C */
> > case 0:
> > /* ... */
> > }
> >
> > but can have e.g.:
> >
> > switch (i) {
> > case 0:
> > {
> > int j = 0;
> > /* ... */
> > }
> > }
> >
> > I think Kees' approach of moving such variable declarations to the
> > enclosing block scope is better than adding another nesting block.
>
> Another nesting level would be bad, but I think this is OK:
>
> switch (i) {
> case 0: {
> int j = 0;
> /* ... */
> }
> case 1: {
> void *p = q;
> /* ... */
> }
> }
>
> I can imagine Kees' patch might have a bad effect on stack consumption,
> unless GCC can be relied on to be smart enough to notice the
> non-overlapping liveness of the vriables and use the same stack slots
> for both.
GCC is reasonable at this. The main issue, though, was most of these
places were using the variables in multiple case statements, so they
couldn't be limited to a single block (or they'd need to be manually
repeated in each block, which is even more ugly, IMO).
Whatever the consensus, I'm happy to tweak the patch.
Thanks!
--
Kees Cook
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 00/27] x86: PIE support and option to extend KASLR randomization
On Fri, Feb 1, 2019 at 8:28 AM Thomas Garnier wrote: > These patches make the changes necessary to build the kernel as Position > Independent Executable (PIE) on x86_64. A PIE kernel can be relocated below > the top 2G of the virtual address space. It allows to optionally extend the > KASLR randomization range from 1G to 3G. The chosen range is the one currently > available, future changes will allow the kernel module to have a wider > randomization range. This also lays the groundwork for doing compilation-unit-granularity KASLR, as Kristen has been working on. With PIE working, the relocations are more sane and boot-time reordering becomes possible (or at least, it becomes the same logically as doing the work on modules, etc). -- Kees Cook ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 0/3] gcc-plugins: Introduce stackinit plugin
On Mon, Jan 28, 2019 at 4:12 PM Alexander Popov wrote: > > On 23.01.2019 14:03, Kees Cook wrote: > > This adds a new plugin "stackinit" that attempts to perform unconditional > > initialization of all stack variables > > Hello Kees! Hello everyone! > > I was curious about the performance impact of the initialization of all stack > variables. So I did a very brief test with this plugin on top of 4.20.5. > > hackbench on Intel Core i7-4770 showed ~0.7% slowdown. > hackbench on Kirin 620 (ARM Cortex-A53 Octa-core 1.2GHz) showed ~1.3% > slowdown. Thanks for looking at this! I'll be including my hackbench measurements for the v2 here in a moment. > This test involves the kernel scheduler and allocator. I can't say whether > they > use stack aggressively. Maybe performance tests of other subsystems (e.g. > network subsystem) can show different numbers. Did you try? I haven't found a stable network test yet. If someone can find a reasonable workload, I'd love to hear about it. > I've heard a hypothesis that the initialization of all stack variables would > pollute CPU caches, which is critical for some types of computations. Maybe > some > micro-benchmarks can disprove/confirm that? I kind of think micro-benchmarks aren't so useful because they don't represent a real-world workload. I've heard people talk about SAP-HANA as a good test, but I can't get my hands on it. I wonder if anyone has tried "mysqlslap"? -- Kees Cook ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC v1 02/12] mm/usercopy.c: Prepare check_page_span() for PG_reserved changes
On Wed, Oct 23, 2019 at 10:20:14AM +0200, David Hildenbrand wrote:
> On 22.10.19 19:12, David Hildenbrand wrote:
> > Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
> > change that.
> >
> > Let's make sure that the logic in the function won't change. Once we no
> > longer set these pages to reserved, we can rework this function to
> > perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
> >
> > Cc: Kees Cook
> > Cc: Andrew Morton
> > Cc: Kate Stewart
> > Cc: Allison Randal
> > Cc: "Isaac J. Manjarres"
> > Cc: Qian Cai
> > Cc: Thomas Gleixner
> > Signed-off-by: David Hildenbrand
> > ---
> > mm/usercopy.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index 660717a1ea5c..a3ac4be35cde 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr,
> > unsigned long n,
> > * device memory), or CMA. Otherwise, reject since the object spans
> > * several independently allocated pages.
> > */
> > - is_reserved = PageReserved(page);
> > + is_reserved = PageReserved(page) || is_zone_device_page(page);
> > is_cma = is_migrate_cma_page(page);
> > if (!is_reserved && !is_cma)
> > usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
> > for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
> > page = virt_to_head_page(ptr);
> > - if (is_reserved && !PageReserved(page))
> > + if (is_reserved && !(PageReserved(page) ||
> > +is_zone_device_page(page)))
> > usercopy_abort("spans Reserved and non-Reserved pages",
> >NULL, to_user, 0, n);
> > if (is_cma && !is_migrate_cma_page(page))
> >
>
> @Kees, would it be okay to stop checking against ZONE_DEVICE pages here or
> is there a good rationale behind this?
>
> (I would turn this patch into a simple update of the comment if we agree
> that we don't care)
There has been work to actually remove the page span checks entirely,
but there wasn't consensus on what the right way forward was. I continue
to leaning toward just dropping it entirely, but Matthew Wilcox has some
alternative ideas that could use some further thought/testing.
--
Kees Cook
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RESEND] Spectre-v2 (IBPB/IBRS) and SSBD fixes for 4.4.y
On Tue, Jul 24, 2018 at 3:02 PM, Jiri Kosina wrote: > On Tue, 24 Jul 2018, Srivatsa S. Bhat wrote: > >> However, if you are proposing that you'd like to contribute the enhanced >> PTI/Spectre (upstream) patches from the SLES 4.4 tree to 4.4 stable, and >> have them merged instead of this patch series, then I would certainly >> welcome it! > > I'd in principle love us to push everything back to 4.4, but there are a > few reasons (*) why that's not happening shortly. > > Anyway, to point out explicitly what's really needed for those folks > running 4.4-stable and relying on PTI providing The Real Thing(TM), it's > either a 4.4-stable port of > > > http://kernel.suse.com/cgit/kernel-source/plain/patches.suse/x86-entry-64-use-a-per-cpu-trampoline-stack.patch?id=3428a77b02b1ba03e45d8fc352ec350429f57fc7 > > or making THREADINFO_GFP imply __GFP_ZERO. This is true in Linus's tree now. Should be trivial to backport: https://git.kernel.org/linus/e01e80634ecdd -Kees -- Kees Cook Pixel Security ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RESEND] Spectre-v2 (IBPB/IBRS) and SSBD fixes for 4.4.y
On Thu, Aug 2, 2018 at 12:22 PM, Srivatsa S. Bhat
wrote:
> On 7/26/18 4:09 PM, Kees Cook wrote:
>> On Tue, Jul 24, 2018 at 3:02 PM, Jiri Kosina wrote:
>>> On Tue, 24 Jul 2018, Srivatsa S. Bhat wrote:
>>>
>>>> However, if you are proposing that you'd like to contribute the enhanced
>>>> PTI/Spectre (upstream) patches from the SLES 4.4 tree to 4.4 stable, and
>>>> have them merged instead of this patch series, then I would certainly
>>>> welcome it!
>>>
>>> I'd in principle love us to push everything back to 4.4, but there are a
>>> few reasons (*) why that's not happening shortly.
>>>
>>> Anyway, to point out explicitly what's really needed for those folks
>>> running 4.4-stable and relying on PTI providing The Real Thing(TM), it's
>>> either a 4.4-stable port of
>>>
>>>
>>> http://kernel.suse.com/cgit/kernel-source/plain/patches.suse/x86-entry-64-use-a-per-cpu-trampoline-stack.patch?id=3428a77b02b1ba03e45d8fc352ec350429f57fc7
>>>
>>> or making THREADINFO_GFP imply __GFP_ZERO.
>>
>> This is true in Linus's tree now. Should be trivial to backport:
>> https://git.kernel.org/linus/e01e80634ecdd
>>
>
> Hi Jiri, Kees,
>
> Thank you for suggesting the patch! I have attached the (locally
> tested) 4.4 and 4.9 backports of that patch with this mail. (The
> mainline commit applies cleanly on 4.14).
>
> Greg, could you please consider including them in stable 4.4, 4.9
> and 4.14?
I don't think your v4.9 is sufficient: it leaves the vmapped stack
uncleared. v4.9 needs ca182551857 ("kmemleak: clear stale pointers
from task stacks") included in the backport (really, just adding the
memset()).
Otherwise, yup, looks good.
-Kees
--
Kees Cook
Pixel Security
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/xen: Distribute switch variables for initialization
Variables declared in a switch statement before any case statements
cannot be automatically initialized with compiler instrumentation (as
they are not part of any execution flow). With GCC's proposed automatic
stack variable initialization feature, this triggers a warning (and they
don't get initialized). Clang's automatic stack variable initialization
(via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
doesn't initialize such variables[1]. Note that these warnings (or silent
skipping) happen before the dead-store elimination optimization phase,
so even when the automatic initializations are later elided in favor of
direct initializations, the warnings remain.
To avoid these problems, move such variables into the "case" where
they're used or lift them up into the main function body.
arch/x86/xen/enlighten_pv.c: In function ‘xen_write_msr_safe’:
arch/x86/xen/enlighten_pv.c:904:12: warning: statement will never be executed
[-Wswitch-unreachable]
904 | unsigned which;
|^
[1] https://bugs.llvm.org/show_bug.cgi?id=44916
Signed-off-by: Kees Cook
---
arch/x86/xen/enlighten_pv.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 1f756e8b..789dc12b7962 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -896,14 +896,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
{
int ret;
+#ifdef CONFIG_X86_64
+ unsigned which;
+ u64 base;
+#endif
ret = 0;
switch (msr) {
#ifdef CONFIG_X86_64
- unsigned which;
- u64 base;
-
case MSR_FS_BASE: which = SEGBASE_FS; goto set;
case MSR_KERNEL_GS_BASE:which = SEGBASE_GS_USER; goto set;
case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set;
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/xen: Distribute switch variables for initialization
On Thu, Feb 20, 2020 at 11:33:41AM -0500, Boris Ostrovsky wrote: > > > On 2/20/20 1:37 AM, Jürgen Groß wrote: > > On 20.02.20 07:23, Kees Cook wrote: > >> Variables declared in a switch statement before any case statements > >> cannot be automatically initialized with compiler instrumentation (as > >> they are not part of any execution flow). With GCC's proposed automatic > >> stack variable initialization feature, this triggers a warning (and they > >> don't get initialized). Clang's automatic stack variable initialization > >> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also > >> doesn't initialize such variables[1]. Note that these warnings (or > >> silent > >> skipping) happen before the dead-store elimination optimization phase, > >> so even when the automatic initializations are later elided in favor of > >> direct initializations, the warnings remain. > >> > >> To avoid these problems, move such variables into the "case" where > >> they're used or lift them up into the main function body. > >> > >> arch/x86/xen/enlighten_pv.c: In function ‘xen_write_msr_safe’: > >> arch/x86/xen/enlighten_pv.c:904:12: warning: statement will never be > >> executed [-Wswitch-unreachable] > >> 904 | unsigned which; > >> | ^ > >> > >> [1] https://bugs.llvm.org/show_bug.cgi?id=44916 > >> > >> Signed-off-by: Kees Cook > > > > Reviewed-by: Juergen Gross > > > > Applied to for-linus-5.6. > > (I replaced 'unsigned' with 'unsigned int' to quiet down checkpatch ) Oh, good call. Sorry I missed that! Thanks! -- Kees Cook ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > This series aims to fix almost all remaining fall-through warnings in > > order to enable -Wimplicit-fallthrough for Clang. > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > add multiple break/goto/return/fallthrough statements instead of just > > letting the code fall through to the next case. > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > change[1] is meant to be reverted at some point. So, this patch helps > > to move in that direction. > > > > Something important to mention is that there is currently a discrepancy > > between GCC and Clang when dealing with switch fall-through to empty case > > statements or to cases that only contain a break/continue/return > > statement[2][3][4]. > > Are we sure we want to make this change? Was it discussed before? > > Are there any bugs Clangs puritanical definition of fallthrough helped > find? > > IMVHO compiler warnings are supposed to warn about issues that could > be bugs. Falling through to default: break; can hardly be a bug?! It's certainly a place where the intent is not always clear. I think this makes all the cases unambiguous, and doesn't impact the machine code, since the compiler will happily optimize away any behavioral redundancy. -- Kees Cook
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > This series aims to fix almost all remaining fall-through warnings in > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > letting the code fall through to the next case. > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > to move in that direction. > > > > > > > > Something important to mention is that there is currently a discrepancy > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > case > > > > statements or to cases that only contain a break/continue/return > > > > statement[2][3][4]. > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > find? > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > It's certainly a place where the intent is not always clear. I think > > this makes all the cases unambiguous, and doesn't impact the machine > > code, since the compiler will happily optimize away any behavioral > > redundancy. > > If none of the 140 patches here fix a real bug, and there is no change > to machine code then it sounds to me like a W=2 kind of a warning. I'd like to avoid splitting common -W options between default and W=2 just based on the compiler. Getting -Wimplicit-fallthrough enabled found plenty of bugs, so making sure it works correctly for both compilers feels justified to me. (This is just a subset of the same C language short-coming.) > I think clang is just being annoying here, but if I'm the only one who > feels this way chances are I'm wrong :) It's being pretty pedantic, but I don't think it's unreasonable to explicitly state how every case ends. GCC's silence for the case of "fall through to a break" doesn't really seem justified. -- Kees Cook
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > This series aims to fix almost all remaining fall-through warnings in > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > letting the code fall through to the next case. > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > to move in that direction. > > > > > > > > Something important to mention is that there is currently a discrepancy > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > case > > > > statements or to cases that only contain a break/continue/return > > > > statement[2][3][4]. > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > find? > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > It's certainly a place where the intent is not always clear. I think > > this makes all the cases unambiguous, and doesn't impact the machine > > code, since the compiler will happily optimize away any behavioral > > redundancy. > > If none of the 140 patches here fix a real bug, and there is no change > to machine code then it sounds to me like a W=2 kind of a warning. FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ -- Kees Cook
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
> >
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
>
> So looks like the bulk of these are:
> switch (x) {
> case 0:
> ++x;
> default:
> break;
> }
>
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
I still think this isn't right -- it's a case statement that runs off
the end without an explicit flow control determination. I think Clang is
right to warn for these, and GCC should also warn.
--
Kees Cook
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote: > Really, no ... something which produces no improvement has no value at > all ... we really shouldn't be wasting maintainer time with it because > it has a cost to merge. I'm not sure we understand where the balance > lies in value vs cost to merge but I am confident in the zero value > case. What? We can't measure how many future bugs aren't introduced because the kernel requires explicit case flow-control statements for all new code. We already enable -Wimplicit-fallthrough globally, so that's not the discussion. The issue is that Clang is (correctly) even more strict than GCC for this, so these are the remaining ones to fix for full Clang coverage too. People have spent more time debating this already than it would have taken to apply the patches. :) This is about robustness and language wrangling. It's a big code-base, and this is the price of our managing technical debt for permanent robustness improvements. (The numbers I ran from Gustavo's earlier patches were that about 10% of the places adjusted were identified as legitimate bugs being fixed. This final series may be lower, but there are still bugs being found from it -- we need to finish this and shut the door on it for good.) -- Kees Cook
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:05:35PM -0800, James Bottomley wrote:
> Now, what we have seems to be about 6 cases (at least what's been shown
> in this thread) where a missing break would cause potentially user
> visible issues. That means the value of this isn't zero, but it's not
> a no-brainer massive win either. That's why I think asking what we've
> invested vs the return isn't a useless exercise.
The number is much higher[1]. If it were 6 in the entire history of the
kernel, I would agree with you. :) Some were fixed _before_ Gustavo's
effort too, which I also count towards the idea of "this is a dangerous
weakness in C, and now we have stopped it forever."
> But the broader point I'm making is just because the compiler people
> come up with a shiny new warning doesn't necessarily mean the problem
> it's detecting is one that causes us actual problems in the code base.
> I'd really be happier if we had a theory about what classes of CVE or
> bug we could eliminate before we embrace the next new warning.
But we did! It was long ago justified and documented[2], and even links to
the CWE[3] for it. This wasn't random joy over discovering a new warning
we could turn on, this was turning on a warning that the compiler folks
finally gave us to handle an entire class of flaws. If we need to update
the code-base to address it not a useful debate -- that was settled
already, even if you're only discovering it now. :P. This last patch
set is about finishing that work for Clang, which is correctly even
more strict than GCC.
-Kees
[1] https://outflux.net/slides/2019/lss/kspp.pdf calls out specific
numbers (about 6.5% of the patches fixed missing breaks):
v4.19: 3 of 129
v4.20: 2 of 59
v5.0: 3 of 56
v5.1: 10 of 100
v5.2: 6 of 71
v5.3: 7 of 69
And in the history of the kernel, it's been an ongoing source of
flaws:
$ l --no-merges | grep -i 'missing break' | wc -l
185
The frequency of such errors being "naturally" found was pretty
steady until the static checkers started warning, and then it was
on the rise, but the full effort flushed the rest out, and now it's
dropped to almost zero:
1 v2.6.12
3 v2.6.16.28
1 v2.6.17
1 v2.6.19
2 v2.6.21
1 v2.6.22
3 v2.6.24
3 v2.6.29
1 v2.6.32
1 v2.6.33
1 v2.6.35
4 v2.6.36
3 v2.6.38
2 v2.6.39
7 v3.0
2 v3.1
2 v3.2
2 v3.3
3 v3.4
1 v3.5
8 v3.6
7 v3.7
3 v3.8
6 v3.9
3 v3.10
2 v3.11
5 v3.12
5 v3.13
2 v3.14
4 v3.15
2 v3.16
3 v3.17
2 v3.18
2 v3.19
1 v4.0
2 v4.1
5 v4.2
4 v4.5
5 v4.7
6 v4.8
1 v4.9
3 v4.10
2 v4.11
6 v4.12
3 v4.13
2 v4.14
5 v4.15
2 v4.16
7 v4.18
2 v4.19
6 v4.20
3 v5.0
12 v5.1
3 v5.2
4 v5.3
2 v5.4
1 v5.8
And the reason it's fully zero, is because we still have the cases we're
cleaning up right now. Even this last one from v5.8 is specifically of
the same type this series addresses:
case 4:
color_index = TrueCModeIndex;
+ break;
default:
return;
}
[2]
https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through
All switch/case blocks must end in one of:
break;
fallthrough;
continue;
goto ;
return [expression];
[3] https://cwe.mitre.org/data/definitions/484.html
--
Kees Cook
[PATCH] xen: Replace lkml.org links with lore
As started by commit 05a5f51ca566 ("Documentation: Replace lkml.org
links with lore"), replace lkml.org links with lore to better use a
single source that's more likely to stay available long-term.
Signed-off-by: Kees Cook
---
drivers/xen/xen-acpi-processor.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index ce8ffb595a46..df7cab870be5 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -3,7 +3,8 @@
* Copyright 2012 by Oracle Inc
* Author: Konrad Rzeszutek Wilk
*
- * This code borrows ideas from https://lkml.org/lkml/2011/11/30/249
+ * This code borrows ideas from
+ *
https://lore.kernel.org/lkml/1322673664-14642-6-git-send-email-konrad.w...@oracle.com
* so many thanks go to Kevin Tian
* and Yu Ke .
*/
--
2.25.1
[PATCH 1/4] x86/entry: Work around Clang __bdos() bug
After expanding bounds checking to use __builtin_dynamic_object_size(),
Clang produces a false positive when building with CONFIG_FORTIFY_SOURCE=y
and CONFIG_UBSAN_BOUNDS=y when operating on an array with a dynamic
offset. Work around this by using a direct assignment of an empty
instance. Avoids this warning:
../include/linux/fortify-string.h:309:4: warning: call to
__write_overflow_field declared with 'warn
ing' attribute: detected write beyond size of field (1st parameter); maybe use
struct_group()? [-Wat
tribute-warning]
__write_overflow_field(p_size_field, size);
^
which was isolated to the memset() call in xen_load_idt().
Note that this looks very much like another bug that was worked around:
https://github.com/ClangBuiltLinux/linux/issues/1592
Cc: Juergen Gross
Cc: Boris Ostrovsky
Cc: Nathan Chancellor
Cc: Nick Desaulniers
Cc: xen-devel@lists.xenproject.org
Cc: l...@lists.linux.dev
Signed-off-by: Kees Cook
---
arch/x86/xen/enlighten_pv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 0ed2e487a693..9b1a58dda935 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -765,6 +765,7 @@ static void xen_load_idt(const struct desc_ptr *desc)
{
static DEFINE_SPINLOCK(lock);
static struct trap_info traps[257];
+ static const struct trap_info zero = { };
unsigned out;
trace_xen_cpu_load_idt(desc);
@@ -774,7 +775,7 @@ static void xen_load_idt(const struct desc_ptr *desc)
memcpy(this_cpu_ptr(&idt_desc), desc, sizeof(idt_desc));
out = xen_convert_trap_info(desc, traps, false);
- memset(&traps[out], 0, sizeof(traps[0]));
+ traps[out] = zero;
xen_mc_flush();
if (HYPERVISOR_set_trap_table(traps))
--
2.34.1
Re: [PATCH][next] xen: Replace one-element array with flexible-array member
On Thu, Feb 02, 2023 at 07:28:23PM -0600, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element array with flexible-array > member in struct xen_page_directory. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > This results in no differences in binary output. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/255 > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v6 11/41] mm: Introduce pte_mkwrite_kernel()
On Sat, Feb 18, 2023 at 01:14:03PM -0800, Rick Edgecombe wrote: > The x86 Control-flow Enforcement Technology (CET) feature includes a new > type of memory called shadow stack. This shadow stack memory has some > unusual properties, which requires some core mm changes to function > properly. > > One of these changes is to allow for pte_mkwrite() to create different > types of writable memory (the existing conventionally writable type and > also the new shadow stack type). Future patches will convert pte_mkwrite() > to take a VMA in order to facilitate this, however there are places in the > kernel where pte_mkwrite() is called outside of the context of a VMA. > These are for kernel memory. So create a new variant called > pte_mkwrite_kernel() and switch the kernel users over to it. Have > pte_mkwrite() and pte_mkwrite_kernel() be the same for now. Future patches > will introduce changes to make pte_mkwrite() take a VMA. > > Only do this for architectures that need it because they call pte_mkwrite() > in arch code without an associated VMA. Since it will only currently be > used in arch code, so do not include it in arch_pgtable_helpers.rst. > > Cc: linux-...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-s...@vger.kernel.org > Cc: xen-devel@lists.xenproject.org > Cc: linux-a...@vger.kernel.org > Cc: linux...@kvack.org > Tested-by: Pengfei Xu > Suggested-by: David Hildenbrand > Signed-off-by: Rick Edgecombe I think it's a little weird that it's the only PTE helper taking a vma, but it does seem like the right approach. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v6 13/41] mm: Make pte_mkwrite() take a VMA
On Sat, Feb 18, 2023 at 01:14:05PM -0800, Rick Edgecombe wrote: > The x86 Control-flow Enforcement Technology (CET) feature includes a new > type of memory called shadow stack. This shadow stack memory has some > unusual properties, which requires some core mm changes to function > properly. > > One of these unusual properties is that shadow stack memory is writable, > but only in limited ways. These limits are applied via a specific PTE > bit combination. Nevertheless, the memory is writable, and core mm code > will need to apply the writable permissions in the typical paths that > call pte_mkwrite(). > > In addition to VM_WRITE, the shadow stack VMA's will have a flag denoting > that they are special shadow stack flavor of writable memory. So make > pte_mkwrite() take a VMA, so that the x86 implementation of it can know to > create regular writable memory or shadow stack memory. > > Apply the same changes for pmd_mkwrite() and huge_pte_mkwrite(). > > No functional change. > > Cc: linux-...@vger.kernel.org > Cc: linux-ker...@vger.kernel.org > Cc: linux-al...@vger.kernel.org > Cc: linux-snps-...@lists.infradead.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-c...@vger.kernel.org > Cc: linux-hexa...@vger.kernel.org > Cc: linux-i...@vger.kernel.org > Cc: loonga...@lists.linux.dev > Cc: linux-m...@lists.linux-m68k.org > Cc: Michal Simek > Cc: Dinh Nguyen > Cc: linux-m...@vger.kernel.org > Cc: openr...@lists.librecores.org > Cc: linux-par...@vger.kernel.org > Cc: linuxppc-...@lists.ozlabs.org > Cc: linux-ri...@lists.infradead.org > Cc: linux-s...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: sparcli...@vger.kernel.org > Cc: linux...@lists.infradead.org > Cc: xen-devel@lists.xenproject.org > Cc: linux-a...@vger.kernel.org > Cc: linux...@kvack.org > Tested-by: Pengfei Xu > Suggested-by: David Hildenbrand > Signed-off-by: Rick Edgecombe I'm not an arch maintainer, but it looks like a correct tree-wide refactor. Reviewed-by: Kees Cook -- Kees Cook
Re: [RFC][PATCH 2/6] x86/power: Inline write_cr[04]()
On Thu, Jan 12, 2023 at 03:31:43PM +0100, Peter Zijlstra wrote: > Since we can't do CALL/RET until GS is restored and CR[04] pinning is > of dubious value in this code path, simply write the stored values. > > Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH][next] xen/pci: Avoid -Wflex-array-member-not-at-end warning
On Tue, Aug 20, 2024 at 06:50:56PM -0600, Gustavo A. R. Silva wrote:
> Use the `DEFINE_RAW_FLEX()` helper for an on-stack definition of
> a flexible structure where the size of the flexible-array member
> is known at compile-time, and refactor the rest of the code,
> accordingly.
>
> So, with this, fix the following warning:
>
> drivers/xen/pci.c:48:55: warning: structure containing a flexible array
> member is not at the end of another structure [-Wflex-array-member-not-at-end]
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/xen/pci.c | 14 +-
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 72d4e3f193af..a2facd8f7e51 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -44,15 +44,11 @@ static int xen_add_device(struct device *dev)
> }
> #endif
> if (pci_seg_supported) {
> - struct {
> - struct physdev_pci_device_add add;
> - uint32_t pxm;
> - } add_ext = {
> - .add.seg = pci_domain_nr(pci_dev->bus),
> - .add.bus = pci_dev->bus->number,
> - .add.devfn = pci_dev->devfn
> - };
> - struct physdev_pci_device_add *add = &add_ext.add;
> + DEFINE_RAW_FLEX(struct physdev_pci_device_add, add, optarr, 1);
> +
> + add->seg = pci_domain_nr(pci_dev->bus);
> + add->bus = pci_dev->bus->number;
> + add->devfn = pci_dev->devfn;
>
> #ifdef CONFIG_ACPI
> acpi_handle handle;
Looks correct to me!
Reviewed-by: Kees Cook
--
Kees Cook
Re: Fwd: UBSAN: index 1 is out of range for type 'xen_netif_rx_sring_entry [1]'
le+0x207/0x260
> > > > > [ 3.687932] cpu_startup_entry+0x1d/0x20
> > > > > [ 3.687934] start_secondary+0x129/0x160
> > > > > [ 3.687939] secondary_startup_64_no_verify+0x17e/0x18b
> > > > > [ 3.687945]
> > > > > [ 3.687946]
> > > > >
> > > > > [ 4.624607] bridge: filtering via arp/ip/ip6tables is no
> > > > > longer available by default. Update your scripts to load
> > > > > br_netfilter if you need this.
> > > > > [ 4.629153] Bridge firewalling registered
> > > > > [ 4.745355] Initializing XFRM netlink socket
> > > > > [ 4.794107] loop8: detected capacity change from 0 to 8
> > > > > [ 7.104544] rfkill: input handler disabled
> > > > > [ 26.445163]
> > > > >
> > > > > [ 26.445171] UBSAN: array-index-out-of-bounds in
> > > > > drivers/net/xen-netfront.c:807:4
> > > > > [ 26.445175] index 109 is out of range for type
> > > > > 'xen_netif_tx_sring_entry [1]'
> > > > > [ 26.445178] CPU: 8 PID: 1729 Comm: sshd Not tainted
> > > > > 6.5.0-rc2-1-generation1 #3
> > > > > [ 26.445180] Hardware name: Intel Corporation
> > > > > W2600CR/W2600CR, BIOS SE5C600.86B.02.06.0007.082420181029
> > > > > 01/13/2022
> > > > > [ 26.445181] Call Trace:
> > > > > [ 26.445185]
> > > > > [ 26.445185] dump_stack_lvl+0x48/0x70
> > > > > [ 26.445185] dump_stack+0x10/0x20
> > > > > [ 26.445200] __ubsan_handle_out_of_bounds+0xc6/0x110
> > > > > [ 26.445206] xennet_start_xmit+0x932/0x990
> > > > > [ 26.445211] dev_hard_start_xmit+0x68/0x1e0
> > > > > [ 26.445216] sch_direct_xmit+0x10b/0x350
> > > > > [ 26.445220] __dev_queue_xmit+0x512/0xda0
> > > > > [ 26.445224] ip_finish_output2+0x261/0x540
> > > > > [ 26.445225] __ip_finish_output+0xb6/0x180
> > > > > [ 26.445225] ip_finish_output+0x29/0x100
> > > > > [ 26.445234] ip_output+0x73/0x120
> > > > > [ 26.445234] ? __pfx_ip_finish_output+0x10/0x10
> > > > > [ 26.445238] ip_local_out+0x61/0x70
> > > > > [ 26.445238] __ip_queue_xmit+0x18d/0x470
> > > > > [ 26.445238] ip_queue_xmit+0x15/0x30
> > > > > [ 26.445238] __tcp_transmit_skb+0xb39/0xcc0
> > > > > [ 26.445238] tcp_write_xmit+0x595/0x1570
> > > > > [ 26.445238] ? _copy_from_iter+0x80/0x4a0
> > > > > [ 26.445256] __tcp_push_pending_frames+0x37/0x110
> > > > > [ 26.445259] tcp_push+0x123/0x190
> > > > > [ 26.445260] tcp_sendmsg_locked+0xafe/0xed0
> > > > > [ 26.445264] tcp_sendmsg+0x2c/0x50
> > > > > [ 26.445268] inet_sendmsg+0x42/0x80
> > > > > [ 26.445268] sock_write_iter+0x160/0x180
> > > > > [ 26.445274] vfs_write+0x397/0x440
> > > > > [ 26.445274] ksys_write+0xc9/0x100
> > > > > [ 26.445274] __x64_sys_write+0x19/0x30
> > > > > [ 26.445274] do_syscall_64+0x5c/0x90
> > > > > [ 26.445287] ? syscall_exit_to_user_mode+0x1b/0x50
> > > > > [ 26.445290] ? do_syscall_64+0x68/0x90
> > > > > [ 26.445290] ? do_syscall_64+0x68/0x90
> > > > > [ 26.445294] ? do_syscall_64+0x68/0x90
> > > > > [ 26.445294] ? syscall_exit_to_user_mode+0x1b/0x50
> > > > > [ 26.445298] ? do_syscall_64+0x68/0x90
> > > > > [ 26.445300] ? exc_page_fault+0x94/0x1b0
> > > > > [ 26.445302] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> > > > > [ 26.445306] RIP: 0033:0x7f26c4c3d473
> > > > > [ 26.445318] Code: 8b 15 21 2a 0e 00 f7 d8 64 89 02 48 c7
> > > > > c0 ff ff ff ff eb b7 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0
> > > > > 75 14 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f
> > > > > 1f 40 00 48 83 ec 28 48 89 54 24 18
> > > > > [ 26.445321] RSP: 002b:7ffdee7b5528 EFLAGS: 0246
> > > > > ORIG_RAX: 0001
> > > > > [ 26.445321] RAX: ffda RBX: 0700
> > > > > RCX: 7f26c4c3d473
> > > > > [ 26.445321] RDX: 0700 RSI: 55567032e230
> > > > > RDI: 0004
> > > > > [ 26.445321] RBP: 555670313d70 R08: fff0
> > > > > R09:
> > > > > [ 26.445321] R10: R11: 0246
> > > > > R12: 55566fcb2768
> > > > > [ 26.445321] R13: R14: 0004
> > > > > R15: 55566fc67a80
> > > > > [ 26.445332]
> > > > > [ 26.445333]
> > > > >
> > > >
> > > > See Bugzilla for the full thread and attached dmesg.
> > > >
> > > > Anyway, I'm adding it to regzbot:
> > > >
> > > > #regzbot introduced: 8446066bf8c1f9f
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=217693
> > > >
> > > > Thanks.
> > > >
> > > > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693
> > >
> > > I doubt it is 8446066bf8c1f9f that causes this. Based on the comment
> > > next to the 'ring[1]' in DEFINE_RING_TYPES() in
> > > include/xen/interface/io/ring.h, this is probably caused/exposed by
> > > commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") in
> > > 6.5-rc1, which causes that array to no longer be a flexible array but an
> > > array with one element, which would cause UBSAN to complain about an
> > > array access past index one. Adding Kees and Gustavo.
> >
> > I agree.
> >
> > >
> > > Unfortunately, it seems this file is vendored from Xen, so I assume it
> > > would need to be fixed there then pulled into Linux:
> > >
> > > https://github.com/xen-project/xen/tree/master/xen/include/public/io/ring.h
> >
> > No, I don't think it will be possible to change this in the Xen tree easily.
> >
> > Especially the public Xen headers are meant to be compatible with a large
> > variety of compilers, including rather old ones.
> >
> > This means that ring[1] can't be easily swapped with ring[], as that would
> > cause compile time errors with some compilers.
> >
> > Just modifying the Linux side header is an option, though, as we don't need
> > the same wide range of supported compilers as Xen.
> >
> > I'll send a patch for that purpose.
>
> Oh, in fact there is a way in Xen to do that correctly. It schould be enough
> to
> use ring[XEN_FLEX_ARRAY_DIM], which will do the right thing.
>
> So I'll write a Xen patch first, after all.
Perfect! I went to go look, and yes, this is good:
/* Define a variable length array (depends on compiler). */
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
#define XEN_FLEX_ARRAY_DIM
#elif defined(__GNUC__)
#define XEN_FLEX_ARRAY_DIM 0
#else
#define XEN_FLEX_ARRAY_DIM 1 /* variable size */
#endif
Be careful, of course, going from [1] to [], if anything is using
sizeof() on the structure.
Thanks for fixing this!
-Kees
--
Kees Cook
Re: [PATCH] ALSA: xen-front: refactor deprecated strncpy
On Thu, Jul 27, 2023 at 09:46:36PM -0700, Elliott Mitchell wrote: > On Thu, Jul 27, 2023 at 09:53:24PM +, Justin Stitt wrote: > > Technically, my patch yields subtly different behavior. The original > > implementation with `strncpy` would fill the entire destination buffer > > with null bytes [3] while `strscpy` will leave the junk, uninitialized > > bytes trailing after the _mandatory_ NUL-termination. So, if somehow > > `pcm->name` or `card->driver/shortname/longname` require this > > NUL-padding behavior then `strscpy_pad` should be used. My > > interpretation, though, is that the aforementioned fields are just fine > > as NUL-terminated strings. Please correct my assumptions if needed and > > I'll send in a v2. > > "uninitialized bytes" => "leak of sensitive information" => "security hole" For xen_snd_front_alsa_init(), "card" is already zero-initialized in snd_card_new(). For new_pcm_instance(), "pcm" is already zero-initialized in _snd_pcm_new(). So things look good to me! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH][next] xen: privcmd: Replace zero-length array with flex-array member and use __counted_by
On Thu, Nov 16, 2023 at 12:54:59PM -0600, Gustavo A. R. Silva wrote:
> Fake flexible arrays (zero-length and one-element arrays) are deprecated,
> and should be replaced by flexible-array members. So, replace
> zero-length array with a flexible-array member in `struct
> privcmd_kernel_ioreq`.
>
> Also annotate array `ports` with `__counted_by()` to 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 via `CONFIG_UBSAN_BOUNDS` (for array
> indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions).
>
> This fixes multiple -Warray-bounds warnings:
> drivers/xen/privcmd.c:1239:30: warning: array subscript i is outside array
> bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
> drivers/xen/privcmd.c:1240:30: warning: array subscript i is outside array
> bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
> drivers/xen/privcmd.c:1241:30: warning: array subscript i is outside array
> bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
> drivers/xen/privcmd.c:1245:33: warning: array subscript i is outside array
> bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
> drivers/xen/privcmd.c:1258:67: warning: array subscript i is outside array
> bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
>
> This results in no differences in binary output.
>
> Signed-off-by: Gustavo A. R. Silva
Looks right to me. I can see the allocation:
size = struct_size(kioreq, ports, ioeventfd->vcpus);
kioreq = kzalloc(size, GFP_KERNEL);
if (!kioreq)
return ERR_PTR(-ENOMEM);
kioreq->dom = ioeventfd->dom;
kioreq->vcpus = ioeventfd->vcpus;
Reviewed-by: Kees Cook
--
Kees Cook
[PATCH 00/32] Introduce flexible array struct memcpy() helpers
Hi, This is the next phase of memcpy() buffer bounds checking[1], which starts by adding a new set of helpers to address common code patterns that result in memcpy() usage that can't be easily verified by the compiler (i.e. dynamic bounds due to flexible arrays). The runtime WARN from memcpy has been posted before, but now there's more context around alternatives for refactoring false positives, etc. The core of this series is patches 2 (flex_array.h), 3 (flex_array KUnit), and 4 (runtime memcpy WARN). Patch 1 is a fix to land before 4 (and I can send separately), and everything else are examples of what the conversions look like for one of the helpers, mem_to_flex_dup(). These will need to land via their respective trees, but they all depend on patch 2, which I'm hoping to land in the coming merge window. I'm happy to also point out that the conversions (patches 5+) are actually a net reduction in lines of code: 49 files changed, 154 insertions(+), 244 deletions(-) Anyway, please let me know what you think. And apologies in advance if this is spammy; the CC list got rather large due to the "treewide" nature of the example conversions. Also available here: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=flexcpy/next-20220502 -Kees [1] https://lwn.net/Articles/864521/ Kees Cook (32): netlink: Avoid memcpy() across flexible array boundary Introduce flexible array struct memcpy() helpers flex_array: Add Kunit tests fortify: Add run-time WARN for cross-field memcpy() brcmfmac: Use mem_to_flex_dup() with struct brcmf_fweh_queue_item iwlwifi: calib: Prepare to use mem_to_flex_dup() iwlwifi: calib: Use mem_to_flex_dup() with struct iwl_calib_result iwlwifi: mvm: Use mem_to_flex_dup() with struct ieee80211_key_conf p54: Use mem_to_flex_dup() with struct p54_cal_database wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg nl80211: Use mem_to_flex_dup() with struct cfg80211_cqm_config cfg80211: Use mem_to_flex_dup() with struct cfg80211_bss_ies mac80211: Use mem_to_flex_dup() with several structs af_unix: Use mem_to_flex_dup() with struct unix_address 802/garp: Use mem_to_flex_dup() with struct garp_attr 802/mrp: Use mem_to_flex_dup() with struct mrp_attr net/flow_offload: Use mem_to_flex_dup() with struct flow_action_cookie firewire: Use __mem_to_flex_dup() with struct iso_interrupt_event afs: Use mem_to_flex_dup() with struct afs_acl ASoC: sigmadsp: Use mem_to_flex_dup() with struct sigmadsp_data soc: qcom: apr: Use mem_to_flex_dup() with struct apr_rx_buf atags_proc: Use mem_to_flex_dup() with struct buffer Bluetooth: Use mem_to_flex_dup() with struct hci_op_configure_data_path IB/hfi1: Use mem_to_flex_dup() for struct tid_rb_node Drivers: hv: utils: Use mem_to_flex_dup() with struct cn_msg ima: Use mem_to_flex_dup() with struct modsig KEYS: Use mem_to_flex_dup() with struct user_key_payload selinux: Use mem_to_flex_dup() with xfrm and sidtab xtensa: Use mem_to_flex_dup() with struct property usb: gadget: f_fs: Use mem_to_flex_dup() with struct ffs_buffer xenbus: Use mem_to_flex_dup() with struct read_buffer esas2r: Use __mem_to_flex() with struct atto_ioctl arch/arm/kernel/atags_proc.c | 12 +- arch/xtensa/platforms/xtfpga/setup.c | 9 +- drivers/firewire/core-cdev.c | 7 +- drivers/hv/hv_utils_transport.c | 7 +- drivers/infiniband/hw/hfi1/user_exp_rcv.c | 7 +- drivers/infiniband/hw/hfi1/user_exp_rcv.h | 4 +- drivers/net/wireless/ath/wcn36xx/smd.c| 8 +- drivers/net/wireless/ath/wcn36xx/smd.h| 4 +- .../broadcom/brcm80211/brcmfmac/fweh.c| 11 +- drivers/net/wireless/intel/iwlwifi/dvm/agn.h | 2 +- .../net/wireless/intel/iwlwifi/dvm/calib.c| 23 +- .../net/wireless/intel/iwlwifi/dvm/ucode.c| 8 +- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +- drivers/net/wireless/intersil/p54/eeprom.c| 8 +- drivers/net/wireless/intersil/p54/p54.h | 4 +- drivers/scsi/esas2r/atioctl.h | 1 + drivers/scsi/esas2r/esas2r_ioctl.c| 11 +- drivers/soc/qcom/apr.c| 12 +- drivers/usb/gadget/function/f_fs.c| 11 +- drivers/xen/xenbus/xenbus_dev_frontend.c | 12 +- fs/afs/internal.h | 4 +- fs/afs/xattr.c| 7 +- include/keys/user-type.h | 4 +- include/linux/flex_array.h| 637 ++ include/linux/fortify-string.h| 70 +- include/linux/of.h| 3 +- include/linux/string.h| 1 + include/net/af_unix.h | 14 +- include/net/bluetooth/hci.h | 4 +- include/net/cfg80211.h| 4 +- include/net/flow_offload.h
[PATCH 02/32] Introduce flexible array struct memcpy() helpers
many of the open-coded
patterns of using memcpy() with a dynamically-sized destination buffer.
[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1990.htm
Cc: "Gustavo A. R. Silva"
Cc: Keith Packard
Cc: Francis Laniel
Cc: Daniel Axtens
Cc: Dan Williams
Cc: Vincenzo Frascino
Cc: Guenter Roeck
Cc: Daniel Vetter
Cc: Tadeusz Struk
Signed-off-by: Kees Cook
---
include/linux/flex_array.h | 637
include/linux/string.h | 1 +
include/uapi/linux/stddef.h | 14 +
3 files changed, 652 insertions(+)
create mode 100644 include/linux/flex_array.h
diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
new file mode 100644
index ..b2cf219f7b56
--- /dev/null
+++ b/include/linux/flex_array.h
@@ -0,0 +1,637 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FLEX_ARRAY_H_
+#define _LINUX_FLEX_ARRAY_H_
+
+#include
+/*
+ * A "flexible array structure" is a struct which ends with a flexible
+ * array _and_ contains a member that represents how many array elements
+ * are present in the flexible array structure:
+ *
+ * struct flex_array_struct_example {
+ * ... // arbitrary members
+ * u16 part_count; // count of elements stored in "parts" below.
+ * .. // arbitrary members
+ * u32 parts[];// flexible array with elements of type u32.
+ * };
+ *
+ * Without the "count of elements" member, a structure ending with a
+ * flexible array has no way to check its own size, and should be
+ * considered just a blob of memory that is length-checked through some
+ * other means. Kernel structures with flexible arrays should strive to
+ * always be true flexible array structures so that they can be operated
+ * on with the flex*()-family of helpers defined below.
+ *
+ * An "encapsulating flexible array structure" is a structure that contains
+ * a full "flexible array structure" as its final struct member. These are
+ * used frequently when needing to pass around a copy of a flexible array
+ * structure, and track other things about the data outside of the scope of
+ * the flexible array structure itself:
+ *
+ * struct encapsulating_example {
+ * ... // other members
+ * struct flex_array_struct_example fas;
+ * };
+ *
+ * For bounds checking operations on a flexible array structure, member
+ * aliases must be created so the helpers can always locate the associated
+ * members. Marking up the examples above would look like this:
+ *
+ * struct flex_array_struct_example {
+ * ... // arbitrary members
+ * // count of elements stored in "parts" below.
+ * DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u16, part_count);
+ * .. // arbitrary members
+ * // flexible array with elements of type u32.
+ * DECLARE_FLEX_ARRAY_ELEMENTS(u32, parts);
+ * };
+ *
+ * The above creates the aliases for part_count as __flex_array_elements_count
+ * and parts as __flex_array_elements.
+ *
+ * For encapsulated flexible array structs, there are alternative helpers
+ * below where the flexible array struct member name can be explicitly
+ * included as an argument. (See the @dot_fas_member arguments below.)
+ *
+ *
+ * Examples:
+ *
+ * Using mem_to_flex():
+ *
+ *struct single {
+ *u32 flags;
+ *u32 count;
+ *u8 data[];
+ *};
+ *struct single *ptr_single;
+ *
+ *struct encap {
+ *u16 info;
+ *struct single single;
+ *};
+ *struct encap *ptr_encap;
+ *
+ *struct blob {
+ *u32 flags;
+ *u8 data[];
+ *};
+ *
+ *struct split {
+ *u32 count;
+ *struct blob blob;
+ *};
+ *struct split *ptr_split;
+ *
+ *mem_to_flex(ptr_one, src, count);
+ *__mem_to_flex(ptr_encap, single.data, single.count, src, count);
+ *__mem_to_flex(ptr_split, count, blob.data, src, count);
+ *
+ */
+
+/* These are wrappers around the UAPI macros. */
+#define DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(TYPE, NAME) \
+ __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(TYPE, NAME)
+
+#define DECLARE_FLEX_ARRAY_ELEMENTS(TYPE, NAME)
\
+ __DECLARE_FLEX_ARRAY_ELEMENTS(TYPE, NAME)
+
+/* All the helpers return negative on failure, as must be checked. */
+static inline int __must_check __must_check_errno(int err)
+{
+ return err;
+}
+
+/**
+ * __fas_elements_bytes - Calculate potential size of the flexible
+ * array elements of a given flexible array
+ * structure.
+ *
+ * @p: Pointer to flexible array structure.
+ * @flex_member: Member name of the flexible array elements.
+ * @count_member: Member name of the flexible array elements count.
+ * @elements_count: Count of proposed number of @p->__flex_a
[PATCH 11/32] nl80211: Use mem_to_flex_dup() with struct cfg80211_cqm_config
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Johannes Berg
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Cc: Eric Dumazet
Signed-off-by: Kees Cook
---
net/wireless/core.h| 4 ++--
net/wireless/nl80211.c | 15 ---
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 3a7dbd63d8c6..899d111993c6 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -295,8 +295,8 @@ struct cfg80211_beacon_registration {
struct cfg80211_cqm_config {
u32 rssi_hyst;
s32 last_rssi_event_value;
- int n_rssi_thresholds;
- s32 rssi_thresholds[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, n_rssi_thresholds);
+ DECLARE_FLEX_ARRAY_ELEMENTS(s32, rssi_thresholds);
};
void cfg80211_destroy_ifaces(struct cfg80211_registered_device *rdev);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 945ed87d12e0..70df7132cce8 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12096,21 +12096,14 @@ static int nl80211_set_cqm_rssi(struct genl_info
*info,
wdev_lock(wdev);
if (n_thresholds) {
- struct cfg80211_cqm_config *cqm_config;
+ struct cfg80211_cqm_config *cqm_config = NULL;
- cqm_config = kzalloc(struct_size(cqm_config, rssi_thresholds,
-n_thresholds),
-GFP_KERNEL);
- if (!cqm_config) {
- err = -ENOMEM;
+ err = mem_to_flex_dup(&cqm_config, thresholds, n_thresholds,
+ GFP_KERNEL);
+ if (err)
goto unlock;
- }
cqm_config->rssi_hyst = hysteresis;
- cqm_config->n_rssi_thresholds = n_thresholds;
- memcpy(cqm_config->rssi_thresholds, thresholds,
- flex_array_size(cqm_config, rssi_thresholds,
- n_thresholds));
wdev->cqm_config = cqm_config;
}
--
2.32.0
[PATCH 14/32] af_unix: Use mem_to_flex_dup() with struct unix_address
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Kuniyuki Iwashima
Cc: Alexei Starovoitov
Cc: Cong Wang
Cc: Al Viro
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/net/af_unix.h | 14 --
net/unix/af_unix.c| 7 ++-
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index a7ef624ed726..422535b71295 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -25,8 +25,18 @@ extern struct hlist_head unix_socket_table[2 *
UNIX_HASH_SIZE];
struct unix_address {
refcount_t refcnt;
- int len;
- struct sockaddr_un name[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len);
+ union {
+ DECLARE_FLEX_ARRAY(struct sockaddr_un, name);
+ /*
+* While a struct is used to access the flexible
+* array, it may only be partially populated, and
+* "len" above is actually tracking bytes, not a
+* count of struct sockaddr_un elements, so also
+* include a byte-size flexible array.
+*/
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, bytes);
+ };
};
struct unix_skb_parms {
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e1dd9e9c8452..8410cbc82ded 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -244,15 +244,12 @@ EXPORT_SYMBOL_GPL(unix_peer_get);
static struct unix_address *unix_create_addr(struct sockaddr_un *sunaddr,
int addr_len)
{
- struct unix_address *addr;
+ struct unix_address *addr = NULL;
- addr = kmalloc(sizeof(*addr) + addr_len, GFP_KERNEL);
- if (!addr)
+ if (mem_to_flex_dup(&addr, sunaddr, addr_len, GFP_KERNEL))
return NULL;
refcount_set(&addr->refcnt, 1);
- addr->len = addr_len;
- memcpy(addr->name, sunaddr, addr_len);
return addr;
}
--
2.32.0
[PATCH 01/32] netlink: Avoid memcpy() across flexible array boundary
In preparation for run-time memcpy() bounds checking, split the nlmsg
copying for error messages (which crosses a previous unspecified flexible
array boundary) in half. Avoids the future run-time warning:
memcpy: detected field-spanning write (size 32) of single field "&errmsg->msg"
(size 16)
Creates an explicit flexible array at the end of nlmsghdr for the payload,
named "nlmsg_payload". There is no impact on UAPI; the sizeof(struct
nlmsghdr) does not change, but now the compiler can better reason about
where things are being copied.
Fixed-by: Rasmus Villemoes
Link:
https://lore.kernel.org/lkml/d7251d92-150b-5346-6237-52afc154b...@rasmusvillemoes.dk
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Rich Felker
Cc: Eric Dumazet
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 5 -
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 855dffb4c1c3..47f9342d51bc 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -47,6 +47,7 @@ struct nlmsghdr {
__u16 nlmsg_flags;/* Additional flags */
__u32 nlmsg_seq; /* Sequence number */
__u32 nlmsg_pid; /* Sending process port ID */
+ __u8nlmsg_payload[];/* Contents of message */
};
/* Flags values */
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 1b5a9c2e1c29..09346aee1022 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr
*nlh, int err,
NLMSG_ERROR, payload, flags);
errmsg = nlmsg_data(rep);
errmsg->error = err;
- memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len :
sizeof(*nlh));
+ errmsg->msg = *nlh;
+ if (payload > sizeof(*errmsg))
+ memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload,
+ nlh->nlmsg_len - sizeof(*nlh));
if (nlk_has_extack && extack) {
if (extack->_msg) {
--
2.32.0
[PATCH 20/32] ASoC: sigmadsp: Use mem_to_flex_dup() with struct sigmadsp_data
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Lars-Peter Clausen
Cc: "Nuno Sá"
Cc: Liam Girdwood
Cc: Mark Brown
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: alsa-de...@alsa-project.org
Signed-off-by: Kees Cook
---
sound/soc/codecs/sigmadsp.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c
index b992216aee55..648bdc73c5d9 100644
--- a/sound/soc/codecs/sigmadsp.c
+++ b/sound/soc/codecs/sigmadsp.c
@@ -42,8 +42,8 @@ struct sigmadsp_data {
struct list_head head;
uint32_t samplerates;
unsigned int addr;
- unsigned int length;
- uint8_t data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned int, length);
+ DECLARE_FLEX_ARRAY_ELEMENTS(uint8_t, data);
};
struct sigma_fw_chunk {
@@ -263,7 +263,7 @@ static int sigma_fw_load_data(struct sigmadsp *sigmadsp,
const struct sigma_fw_chunk *chunk, unsigned int length)
{
const struct sigma_fw_chunk_data *data_chunk;
- struct sigmadsp_data *data;
+ struct sigmadsp_data *data = NULL;
if (length <= sizeof(*data_chunk))
return -EINVAL;
@@ -272,14 +272,11 @@ static int sigma_fw_load_data(struct sigmadsp *sigmadsp,
length -= sizeof(*data_chunk);
- data = kzalloc(sizeof(*data) + length, GFP_KERNEL);
- if (!data)
+ if (mem_to_flex_dup(&data, data_chunk->data, length, GFP_KERNEL))
return -ENOMEM;
data->addr = le16_to_cpu(data_chunk->addr);
- data->length = length;
data->samplerates = le32_to_cpu(chunk->samplerates);
- memcpy(data->data, data_chunk->data, length);
list_add_tail(&data->head, &sigmadsp->data_list);
return 0;
--
2.32.0
[PATCH 07/32] iwlwifi: calib: Use mem_to_flex_dup() with struct iwl_calib_result
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Avoids future false-positive warning when strict run-time memcpy()
bounds checking is enabled:
memcpy: detected field-spanning write (size 8) of single field "&res->hdr"
(size 4)
Adds an additional size check since the minimum isn't 0.
Reported-by: Andy Lavr
Cc: Luca Coelho
Cc: Kalle Valo
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Gregory Greenman
Cc: Eric Dumazet
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/intel/iwlwifi/dvm/calib.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
b/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
index ae1f0cf560e2..7480c19d7af0 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
@@ -18,8 +18,11 @@
/* Opaque calibration results */
struct iwl_calib_result {
struct list_head list;
- size_t cmd_len;
- struct iwl_calib_cmd cmd;
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, cmd_len);
+ union {
+ struct iwl_calib_cmd cmd;
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
+ };
/* data follows */
};
@@ -59,14 +62,10 @@ int iwl_send_calib_results(struct iwl_priv *priv)
int iwl_calib_set(struct iwl_priv *priv,
const struct iwl_calib_cmd *cmd, int len)
{
- struct iwl_calib_result *res, *tmp;
+ struct iwl_calib_result *res = NULL, *tmp;
- res = kmalloc(sizeof(*res) + len - sizeof(struct iwl_calib_hdr),
- GFP_ATOMIC);
- if (!res)
+ if (len < sizeof(*cmd) || mem_to_flex_dup(&res, cmd, len, GFP_ATOMIC))
return -ENOMEM;
- memcpy(&res->hdr, cmd, len);
- res->cmd_len = len;
list_for_each_entry(tmp, &priv->calib_results, list) {
if (tmp->cmd.hdr.op_code == res->cmd.hdr.op_code) {
--
2.32.0
[PATCH 03/32] flex_array: Add Kunit tests
Add tests for the new flexible array structure helpers. These can be run
with:
make ARCH=um mrproper
./tools/testing/kunit/kunit.py config
./tools/testing/kunit/kunit.py run flex_array
Cc: David Gow
Cc: kunit-...@googlegroups.com
Signed-off-by: Kees Cook
---
lib/Kconfig.debug | 12 +-
lib/Makefile | 1 +
lib/flex_array_kunit.c | 523 +
3 files changed, 531 insertions(+), 5 deletions(-)
create mode 100644 lib/flex_array_kunit.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9077bb38bc93..8bae6b169c50 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST
Builds unit tests for the check_*_overflow(), size_*(), allocation,
and
related functions.
- For more information on KUnit and unit tests in general please refer
- to the KUnit documentation in Documentation/dev-tools/kunit/.
-
- If unsure, say N.
-
config STACKINIT_KUNIT_TEST
tristate "Test level of stack variable initialization" if
!KUNIT_ALL_TESTS
depends on KUNIT
@@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST
CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
+config FLEX_ARRAY_KUNIT_TEST
+ tristate "Test flex_*() family of helper functions at runtime" if
!KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ Builds unit tests for flexible array copy helper functions.
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index 6b9ffc1bd1ee..9884318db330 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -366,6 +366,7 @@ obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
+obj-$(CONFIG_FLEX_ARRAY_KUNIT_TEST) += flex_array_kunit.o
obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/flex_array_kunit.c b/lib/flex_array_kunit.c
new file mode 100644
index ..48bee88945b4
--- /dev/null
+++ b/lib/flex_array_kunit.c
@@ -0,0 +1,523 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for flex_*() array manipulation helpers.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+
+#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B)do {\
+ STRUCT_A *ptr_A;\
+ STRUCT_B *ptr_B;\
+ int rc; \
+ size_t size_A, size_B; \
+ \
+ /* matching types for flex array elements and count */ \
+ KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \
+ KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \
+ *ptr_B->__flex_array_elements));\
+ KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \
+ ptr_B->__flex_array_elements_count)); \
+ KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \
+ sizeof(*ptr_B->__flex_array_elements)); \
+ KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \
+ offsetof(typeof(*ptr_B), \
+ __flex_array_elements)); \
+ KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen),\
+ offsetof(typeof(*ptr_B), \
+ __flex_array_elements_count)); \
+ \
+ /* struct_size() vs __fas_bytes() */\
+ size_A = struct_size(ptr_A, data, 13); \
+ rc = __fas_bytes(ptr_B, __flex_array_elements, \
+__flex_array_elements_count, 13, &size_B); \
+ KUNIT_EXPECT_EQ(test, rc, 0); \
+ KUNIT_EXPECT_EQ(test, size_A, size_B); \
+ \
+ /* flex_array_size() vs __fas_elements_bytes() */ \
+ size_A = flex_array_size(ptr_A, data, 13); \
+ rc = __fas_elements_bytes(ptr_B, __flex_array_elements, \
+__flex_array_elements_count, 13, &size_B); \
+ KUNIT_EXPECT_EQ(test, r
[PATCH 04/32] fortify: Add run-time WARN for cross-field memcpy()
Enable run-time checking of dynamic memcpy() and memmove() lengths,
issuing a WARN when a write would exceed the size of the target struct
member, when built with CONFIG_FORTIFY_SOURCE=y. This would have caught
all of the memcpy()-based buffer overflows from 2018 through 2020,
specifically covering all the cases where the destination buffer size
is known at compile time.
This change ONLY adds a run-time warning. As false positives are currently
still expected, this will not block the overflow. The new warnings will
look like this:
memcpy: detected field-spanning write (size N) of single field "var->dest"
(size M)
WARNING: CPU: n PID: at source/file/path.c:nr function+0xXX/0xXX [module]
The false positives are most likely where intentional field-spanning
writes are happening. These need to be addressed similarly to how the
compile-time cases were addressed: add a struct_group(), split the
memcpy(), use a flex_array.h helper, or some other refactoring.
In order to make identifying/investigating instances of added runtime
checks easier, each instance includes the destination variable name as a
WARN argument, prefixed with 'field "'. Therefore, on any given build,
it is trivial to inspect the artifacts to find instances. For example
on an x86_64 defconfig build, there are 78 new run-time memcpy() bounds
checks added:
$ for i in vmlinux $(find . -name '*.ko'); do \
strings "$i" | grep '^field "'; done | wc -l
78
Currently, the common case where a destination buffer is known to be a
dynamic size (i.e. has a trailing flexible array) does not generate a
WARN. For example:
struct normal_flex_array {
void *a;
int b;
size_t array_size;
u32 c;
u8 flex_array[];
};
struct normal_flex_array *instance;
...
/* These cases will be ignored for run-time bounds checking. */
memcpy(instance, src, len);
memcpy(instance->flex_array, src, len);
This code pattern will need to be addressed separately, likely by
migrating to one of the flex_array.h family of helpers.
Note that one of the dynamic-sized destination cases is irritatingly
unable to be detected by the compiler: when using memcpy() to target
a composite struct member which contains a trailing flexible array
struct. For example:
struct wrapper {
int foo;
char bar;
struct normal_flex_array embedded;
};
struct wrapper *instance;
...
/* This will incorrectly WARN when len > sizeof(instance->embedded) */
memcpy(&instance->embedded, src, len);
These cases end up appearing to the compiler to be sized as if the
flexible array had 0 elements. :( For more details see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
https://godbolt.org/z/vW6x8vh4P
Regardless, all cases of copying to/from flexible array structures
should be migrated to using the new flex*()-family of helpers to gain
their added safety checking, but priority will need to be given to the
"composite flexible array structure destination" cases noted above.
As mentioned, none of these bounds checks block any overflows
currently. For users that have tested their workloads, do not encounter
any warnings, and wish to make these checks stop any overflows, they
can use a big hammer and set the sysctl panic_on_warn=1.
Cc: Nathan Chancellor
Cc: Nick Desaulniers
Cc: Tom Rix
Cc: linux-harden...@vger.kernel.org
Cc: l...@lists.linux.dev
Signed-off-by: Kees Cook
---
include/linux/fortify-string.h | 70 --
1 file changed, 67 insertions(+), 3 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 295637a66c46..9f65527fff40 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -3,6 +3,7 @@
#define _LINUX_FORTIFY_STRING_H_
#include
+#include
#define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
#define __RENAME(x) __asm__(#x)
@@ -303,7 +304,7 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t
size,
* V = vulnerable to run-time overflow (will need refactoring to solve)
*
*/
-__FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
+__FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
const size_t p_size,
const size_t q_size,
const size_t p_size_field,
@@ -352,16 +353,79 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t
size,
if ((p_size != (size_t)(-1) && p_size < size) ||
(q_size != (size_t)(-1) && q_size < size))
fortify_panic(func);
+
+ /*
+* Warn when writing beyond destination field size.
+*
+* We must ignore p_size_field == 0 and -1 for existing
+* 0-element and flexible arrays, until they
[PATCH 06/32] iwlwifi: calib: Prepare to use mem_to_flex_dup()
In preparation for replacing an open-coded memcpy() of a dynamically
side buffer, rearrange the structures to pass enough information into
the calling function to examine the bounds of the struct.
Rearrange the argument passing to use "cmd", rather than "hdr", since
"res" expects to operate on the "data" flex array in "cmd" (that follows
"hdr").
Cc: Luca Coelho
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Lee Jones
Cc: Johannes Berg
Cc: Gregory Greenman
Cc: Kalle Valo
Cc: Eric Dumazet
Cc: Paolo Abeni
Cc: Andy Lavr
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/intel/iwlwifi/dvm/agn.h | 2 +-
drivers/net/wireless/intel/iwlwifi/dvm/calib.c | 10 +-
drivers/net/wireless/intel/iwlwifi/dvm/ucode.c | 8
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/agn.h
b/drivers/net/wireless/intel/iwlwifi/dvm/agn.h
index abb8696ba294..744e111d2ea3 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/agn.h
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/agn.h
@@ -112,7 +112,7 @@ int iwl_load_ucode_wait_alive(struct iwl_priv *priv,
enum iwl_ucode_type ucode_type);
int iwl_send_calib_results(struct iwl_priv *priv);
int iwl_calib_set(struct iwl_priv *priv,
- const struct iwl_calib_hdr *cmd, int len);
+ const struct iwl_calib_cmd *cmd, int len);
void iwl_calib_free_results(struct iwl_priv *priv);
int iwl_dump_nic_event_log(struct iwl_priv *priv, bool full_log,
char **buf);
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
b/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
index a11884fa254b..ae1f0cf560e2 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/calib.c
@@ -19,7 +19,7 @@
struct iwl_calib_result {
struct list_head list;
size_t cmd_len;
- struct iwl_calib_hdr hdr;
+ struct iwl_calib_cmd cmd;
/* data follows */
};
@@ -43,12 +43,12 @@ int iwl_send_calib_results(struct iwl_priv *priv)
int ret;
hcmd.len[0] = res->cmd_len;
- hcmd.data[0] = &res->hdr;
+ hcmd.data[0] = &res->cmd;
hcmd.dataflags[0] = IWL_HCMD_DFL_NOCOPY;
ret = iwl_dvm_send_cmd(priv, &hcmd);
if (ret) {
IWL_ERR(priv, "Error %d on calib cmd %d\n",
- ret, res->hdr.op_code);
+ ret, res->cmd.hdr.op_code);
return ret;
}
}
@@ -57,7 +57,7 @@ int iwl_send_calib_results(struct iwl_priv *priv)
}
int iwl_calib_set(struct iwl_priv *priv,
- const struct iwl_calib_hdr *cmd, int len)
+ const struct iwl_calib_cmd *cmd, int len)
{
struct iwl_calib_result *res, *tmp;
@@ -69,7 +69,7 @@ int iwl_calib_set(struct iwl_priv *priv,
res->cmd_len = len;
list_for_each_entry(tmp, &priv->calib_results, list) {
- if (tmp->hdr.op_code == res->hdr.op_code) {
+ if (tmp->cmd.hdr.op_code == res->cmd.hdr.op_code) {
list_replace(&tmp->list, &res->list);
kfree(tmp);
return 0;
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c
b/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c
index 4b27a53d0bb4..bb13ca5d666c 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c
@@ -356,18 +356,18 @@ static bool iwlagn_wait_calib(struct iwl_notif_wait_data
*notif_wait,
struct iwl_rx_packet *pkt, void *data)
{
struct iwl_priv *priv = data;
- struct iwl_calib_hdr *hdr;
+ struct iwl_calib_cmd *cmd;
if (pkt->hdr.cmd != CALIBRATION_RES_NOTIFICATION) {
WARN_ON(pkt->hdr.cmd != CALIBRATION_COMPLETE_NOTIFICATION);
return true;
}
- hdr = (struct iwl_calib_hdr *)pkt->data;
+ cmd = (struct iwl_calib_cmd *)pkt->data;
- if (iwl_calib_set(priv, hdr, iwl_rx_packet_payload_len(pkt)))
+ if (iwl_calib_set(priv, cmd, iwl_rx_packet_payload_len(pkt)))
IWL_ERR(priv, "Failed to record calibration data %d\n",
- hdr->op_code);
+ cmd->hdr.op_code);
return false;
}
--
2.32.0
[PATCH 12/32] cfg80211: Use mem_to_flex_dup() with struct cfg80211_bss_ies
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Johannes Berg
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Eric Dumazet
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/net/cfg80211.h | 4 ++--
net/wireless/scan.c| 21 ++---
2 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 68713388b617..fa236015f6ef 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2600,9 +2600,9 @@ struct cfg80211_inform_bss {
struct cfg80211_bss_ies {
u64 tsf;
struct rcu_head rcu_head;
- int len;
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len);
bool from_beacon;
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
/**
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 4a6d86432910..9f53d05c6aaa 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1932,7 +1932,7 @@ cfg80211_inform_single_bss_data(struct wiphy *wiphy,
gfp_t gfp)
{
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
- struct cfg80211_bss_ies *ies;
+ struct cfg80211_bss_ies *ies = NULL;
struct ieee80211_channel *channel;
struct cfg80211_internal_bss tmp = {}, *res;
int bss_type;
@@ -1978,13 +1978,10 @@ cfg80211_inform_single_bss_data(struct wiphy *wiphy,
* override the IEs pointer should we have received an earlier
* indication of Probe Response data.
*/
- ies = kzalloc(sizeof(*ies) + ielen, gfp);
- if (!ies)
+ if (mem_to_flex_dup(&ies, ie, ielen, gfp))
return NULL;
- ies->len = ielen;
ies->tsf = tsf;
ies->from_beacon = false;
- memcpy(ies->data, ie, ielen);
switch (ftype) {
case CFG80211_BSS_FTYPE_BEACON:
@@ -2277,7 +2274,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy,
size_t ielen = len - offsetof(struct ieee80211_mgmt,
u.probe_resp.variable);
size_t new_ie_len;
- struct cfg80211_bss_ies *new_ies;
+ struct cfg80211_bss_ies *new_ies = NULL;
const struct cfg80211_bss_ies *old;
u8 cpy_len;
@@ -2314,8 +2311,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy,
if (!new_ie)
return;
- new_ies = kzalloc(sizeof(*new_ies) + new_ie_len, GFP_ATOMIC);
- if (!new_ies)
+ if (mem_to_flex_dup(&new_ies, new_ie, new_ie_len, GFP_ATOMIC))
goto out_free;
pos = new_ie;
@@ -2333,10 +2329,8 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy,
memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len)));
/* update ie */
- new_ies->len = new_ie_len;
new_ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp);
new_ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control);
- memcpy(new_ies->data, new_ie, new_ie_len);
if (ieee80211_is_probe_resp(mgmt->frame_control)) {
old = rcu_access_pointer(nontrans_bss->proberesp_ies);
rcu_assign_pointer(nontrans_bss->proberesp_ies, new_ies);
@@ -2363,7 +2357,7 @@ cfg80211_inform_single_bss_frame_data(struct wiphy *wiphy,
gfp_t gfp)
{
struct cfg80211_internal_bss tmp = {}, *res;
- struct cfg80211_bss_ies *ies;
+ struct cfg80211_bss_ies *ies = NULL;
struct ieee80211_channel *channel;
bool signal_valid;
struct ieee80211_ext *ext = NULL;
@@ -2442,14 +2436,11 @@ cfg80211_inform_single_bss_frame_data(struct wiphy
*wiphy,
capability = le16_to_cpu(mgmt->u.probe_resp.capab_info);
}
- ies = kzalloc(sizeof(*ies) + ielen, gfp);
- if (!ies)
+ if (mem_to_flex_dup(&ies, variable, ielen, gfp))
return NULL;
- ies->len = ielen;
ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp);
ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control) ||
ieee80211_is_s1g_beacon(mgmt->frame_control);
- memcpy(ies->data, variable, ielen);
if (ieee80211_is_probe_resp(mgmt->frame_control))
rcu_assign_pointer(tmp.pub.proberesp_ies, ies);
--
2.32.0
[PATCH 27/32] KEYS: Use mem_to_flex_dup() with struct user_key_payload
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: David Howells
Cc: Jarkko Sakkinen
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: keyri...@vger.kernel.org
Cc: linux-security-mod...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/keys/user-type.h | 4 ++--
security/keys/user_defined.c | 7 ++-
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index 386c31432789..4e67ff902a32 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -26,8 +26,8 @@
*/
struct user_key_payload {
struct rcu_head rcu;/* RCU destructor */
- unsigned short datalen;/* length of this data */
- chardata[] __aligned(__alignof__(u64)); /* actual data */
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned short, datalen);
+ DECLARE_FLEX_ARRAY_ELEMENTS(char, data) __aligned(__alignof__(u64));
};
extern struct key_type key_type_user;
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 749e2a4dcb13..2fb84894cdaa 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -58,21 +58,18 @@ EXPORT_SYMBOL_GPL(key_type_logon);
*/
int user_preparse(struct key_preparsed_payload *prep)
{
- struct user_key_payload *upayload;
+ struct user_key_payload *upayload = NULL;
size_t datalen = prep->datalen;
if (datalen <= 0 || datalen > 32767 || !prep->data)
return -EINVAL;
- upayload = kmalloc(sizeof(*upayload) + datalen, GFP_KERNEL);
- if (!upayload)
+ if (mem_to_flex_dup(&upayload, prep->data, datalen, GFP_KERNEL))
return -ENOMEM;
/* attach the data */
prep->quotalen = datalen;
prep->payload.data[0] = upayload;
- upayload->datalen = datalen;
- memcpy(upayload->data, prep->data, datalen);
return 0;
}
EXPORT_SYMBOL_GPL(user_preparse);
--
2.32.0
[PATCH 28/32] selinux: Use mem_to_flex_dup() with xfrm and sidtab
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying:
struct xfrm_sec_ctx
struct sidtab_str_cache
Cc: Steffen Klassert
Cc: Herbert Xu
Cc: "David S. Miller"
Cc: Paul Moore
Cc: Stephen Smalley
Cc: Eric Paris
Cc: Nick Desaulniers
Cc: Xiu Jianfeng
Cc: "Christian Göttsche"
Cc: net...@vger.kernel.org
Cc: seli...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/uapi/linux/xfrm.h| 4 ++--
security/selinux/ss/sidtab.c | 9 +++--
security/selinux/xfrm.c | 7 ++-
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 65e13a099b1a..4a6fa2beff6a 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -31,9 +31,9 @@ struct xfrm_id {
struct xfrm_sec_ctx {
__u8ctx_doi;
__u8ctx_alg;
- __u16 ctx_len;
+ __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len);
__u32 ctx_sid;
- charctx_str[0];
+ __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str);
};
/* Security Context Domains of Interpretation */
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index a54b8652bfb5..a9d434e8cff7 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -23,8 +23,8 @@ struct sidtab_str_cache {
struct rcu_head rcu_member;
struct list_head lru_member;
struct sidtab_entry *parent;
- u32 len;
- char str[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(char, str);
};
#define index_to_sid(index) ((index) + SECINITSID_NUM + 1)
@@ -570,8 +570,7 @@ void sidtab_sid2str_put(struct sidtab *s, struct
sidtab_entry *entry,
goto out_unlock;
}
- cache = kmalloc(struct_size(cache, str, str_len), GFP_ATOMIC);
- if (!cache)
+ if (mem_to_flex_dup(&cache, str, str_len, GFP_ATOMIC))
goto out_unlock;
if (s->cache_free_slots == 0) {
@@ -584,8 +583,6 @@ void sidtab_sid2str_put(struct sidtab *s, struct
sidtab_entry *entry,
s->cache_free_slots--;
}
cache->parent = entry;
- cache->len = str_len;
- memcpy(cache->str, str, str_len);
list_add(&cache->lru_member, &s->cache_lru_list);
rcu_assign_pointer(entry->cache, cache);
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index c576832febc6..bc7a54bf8f0d 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -345,7 +345,7 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
struct xfrm_sec_ctx *polsec, u32 secid)
{
int rc;
- struct xfrm_sec_ctx *ctx;
+ struct xfrm_sec_ctx *ctx = NULL;
char *ctx_str = NULL;
u32 str_len;
@@ -360,8 +360,7 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
if (rc)
return rc;
- ctx = kmalloc(struct_size(ctx, ctx_str, str_len), GFP_ATOMIC);
- if (!ctx) {
+ if (mem_to_flex_dup(&ctx, ctx_str, str_len, GFP_ATOMIC)) {
rc = -ENOMEM;
goto out;
}
@@ -369,8 +368,6 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
ctx->ctx_doi = XFRM_SC_DOI_LSM;
ctx->ctx_alg = XFRM_SC_ALG_SELINUX;
ctx->ctx_sid = secid;
- ctx->ctx_len = str_len;
- memcpy(ctx->ctx_str, ctx_str, str_len);
x->security = ctx;
atomic_inc(&selinux_xfrm_refcount);
--
2.32.0
[PATCH 10/32] wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Loic Poulain
Cc: Kalle Valo
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: wcn3...@lists.infradead.org
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/ath/wcn36xx/smd.c | 8 ++--
drivers/net/wireless/ath/wcn36xx/smd.h | 4 ++--
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c
b/drivers/net/wireless/ath/wcn36xx/smd.c
index dc3805609284..106af0a2ffc4 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -3343,7 +3343,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
const struct wcn36xx_hal_msg_header *msg_header = buf;
struct ieee80211_hw *hw = priv;
struct wcn36xx *wcn = hw->priv;
- struct wcn36xx_hal_ind_msg *msg_ind;
+ struct wcn36xx_hal_ind_msg *msg_ind = NULL;
wcn36xx_dbg_dump(WCN36XX_DBG_SMD_DUMP, "SMD <<< ", buf, len);
switch (msg_header->msg_type) {
@@ -3407,16 +3407,12 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
case WCN36XX_HAL_PRINT_REG_INFO_IND:
case WCN36XX_HAL_SCAN_OFFLOAD_IND:
- msg_ind = kmalloc(struct_size(msg_ind, msg, len), GFP_ATOMIC);
- if (!msg_ind) {
+ if (mem_to_flex_dup(&msg_ind, buf, len, GFP_ATOMIC)) {
wcn36xx_err("Run out of memory while handling SMD_EVENT
(%d)\n",
msg_header->msg_type);
return -ENOMEM;
}
- msg_ind->msg_len = len;
- memcpy(msg_ind->msg, buf, len);
-
spin_lock(&wcn->hal_ind_lock);
list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h
b/drivers/net/wireless/ath/wcn36xx/smd.h
index 3fd598ac2a27..76ecac46f36b 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -46,8 +46,8 @@ struct wcn36xx_fw_msg_status_rsp {
struct wcn36xx_hal_ind_msg {
struct list_head list;
- size_t msg_len;
- u8 msg[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, msg_len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, msg);
};
struct wcn36xx;
--
2.32.0
[PATCH 09/32] p54: Use mem_to_flex_dup() with struct p54_cal_database
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Christian Lamparter
Cc: Kalle Valo
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/intersil/p54/eeprom.c | 8 ++--
drivers/net/wireless/intersil/p54/p54.h| 4 ++--
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/intersil/p54/eeprom.c
b/drivers/net/wireless/intersil/p54/eeprom.c
index 5bd35c147e19..bd9b3ea327b9 100644
--- a/drivers/net/wireless/intersil/p54/eeprom.c
+++ b/drivers/net/wireless/intersil/p54/eeprom.c
@@ -702,7 +702,7 @@ static int p54_convert_output_limits(struct ieee80211_hw
*dev,
static struct p54_cal_database *p54_convert_db(struct pda_custom_wrapper *src,
size_t total_len)
{
- struct p54_cal_database *dst;
+ struct p54_cal_database *dst = NULL;
size_t payload_len, entries, entry_size, offset;
payload_len = le16_to_cpu(src->len);
@@ -713,16 +713,12 @@ static struct p54_cal_database *p54_convert_db(struct
pda_custom_wrapper *src,
(payload_len + sizeof(*src) != total_len))
return NULL;
- dst = kmalloc(sizeof(*dst) + payload_len, GFP_KERNEL);
- if (!dst)
+ if (mem_to_flex_dup(&dst, src->data, payload_len, GFP_KERNEL))
return NULL;
dst->entries = entries;
dst->entry_size = entry_size;
dst->offset = offset;
- dst->len = payload_len;
-
- memcpy(dst->data, src->data, payload_len);
return dst;
}
diff --git a/drivers/net/wireless/intersil/p54/p54.h
b/drivers/net/wireless/intersil/p54/p54.h
index 3356ea708d81..22bbb6d28245 100644
--- a/drivers/net/wireless/intersil/p54/p54.h
+++ b/drivers/net/wireless/intersil/p54/p54.h
@@ -125,8 +125,8 @@ struct p54_cal_database {
size_t entries;
size_t entry_size;
size_t offset;
- size_t len;
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
#define EEPROM_READBACK_LEN 0x3fc
--
2.32.0
[PATCH 18/32] firewire: Use __mem_to_flex_dup() with struct iso_interrupt_event
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Stefan Richter
Cc: linux1394-de...@lists.sourceforge.net
Signed-off-by: Kees Cook
---
drivers/firewire/core-cdev.c | 7 ++-
include/uapi/linux/firewire-cdev.h | 4 ++--
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index c9fe5903725a..7e884c61e12e 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -913,17 +913,14 @@ static void iso_callback(struct fw_iso_context *context,
u32 cycle,
size_t header_length, void *header, void *data)
{
struct client *client = data;
- struct iso_interrupt_event *e;
+ struct iso_interrupt_event *e = NULL;
- e = kmalloc(sizeof(*e) + header_length, GFP_ATOMIC);
- if (e == NULL)
+ if (__mem_to_flex_dup(&e, .interrupt, header, header_length,
GFP_ATOMIC))
return;
e->interrupt.type = FW_CDEV_EVENT_ISO_INTERRUPT;
e->interrupt.closure = client->iso_closure;
e->interrupt.cycle = cycle;
- e->interrupt.header_length = header_length;
- memcpy(e->interrupt.header, header, header_length);
queue_event(client, &e->event, &e->interrupt,
sizeof(e->interrupt) + header_length, NULL, 0);
}
diff --git a/include/uapi/linux/firewire-cdev.h
b/include/uapi/linux/firewire-cdev.h
index 5effa9832802..22c5f59e9dfa 100644
--- a/include/uapi/linux/firewire-cdev.h
+++ b/include/uapi/linux/firewire-cdev.h
@@ -264,8 +264,8 @@ struct fw_cdev_event_iso_interrupt {
__u64 closure;
__u32 type;
__u32 cycle;
- __u32 header_length;
- __u32 header[0];
+ __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u32, header_length);
+ __DECLARE_FLEX_ARRAY_ELEMENTS(__u32, header);
};
/**
--
2.32.0
[PATCH 16/32] 802/mrp: Use mem_to_flex_dup() with struct mrp_attr
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Yang Yingliang
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/net/mrp.h | 4 ++--
net/802/mrp.c | 9 +++--
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/include/net/mrp.h b/include/net/mrp.h
index 1c308c034e1a..211670bb46f2 100644
--- a/include/net/mrp.h
+++ b/include/net/mrp.h
@@ -91,8 +91,8 @@ struct mrp_attr {
struct rb_node node;
enum mrp_applicant_statestate;
u8 type;
- u8 len;
- unsigned char value[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u8, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(unsigned char, value);
};
enum mrp_applications {
diff --git a/net/802/mrp.c b/net/802/mrp.c
index 35e04cc5390c..8b9b2e685a42 100644
--- a/net/802/mrp.c
+++ b/net/802/mrp.c
@@ -257,7 +257,7 @@ static struct mrp_attr *mrp_attr_create(struct
mrp_applicant *app,
const void *value, u8 len, u8 type)
{
struct rb_node *parent = NULL, **p = &app->mad.rb_node;
- struct mrp_attr *attr;
+ struct mrp_attr *attr = NULL;
int d;
while (*p) {
@@ -273,13 +273,10 @@ static struct mrp_attr *mrp_attr_create(struct
mrp_applicant *app,
return attr;
}
}
- attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
- if (!attr)
- return attr;
+ if (mem_to_flex_dup(&attr, value, len, GFP_ATOMIC))
+ return NULL;
attr->state = MRP_APPLICANT_VO;
attr->type = type;
- attr->len = len;
- memcpy(attr->value, value, len);
rb_link_node(&attr->node, parent, p);
rb_insert_color(&attr->node, &app->mad);
--
2.32.0
[PATCH 22/32] atags_proc: Use mem_to_flex_dup() with struct buffer
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Russell King
Cc: Christian Brauner
Cc: Andrew Morton
Cc: Muchun Song
Cc: linux-arm-ker...@lists.infradead.org
Signed-off-by: Kees Cook
---
arch/arm/kernel/atags_proc.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/arm/kernel/atags_proc.c b/arch/arm/kernel/atags_proc.c
index 3ec2afe78423..638bbb616daa 100644
--- a/arch/arm/kernel/atags_proc.c
+++ b/arch/arm/kernel/atags_proc.c
@@ -6,8 +6,8 @@
#include
struct buffer {
- size_t size;
- char data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, size);
+ DECLARE_FLEX_ARRAY_ELEMENTS(char, data);
};
static ssize_t atags_read(struct file *file, char __user *buf,
@@ -38,7 +38,7 @@ static int __init init_atags_procfs(void)
*/
struct proc_dir_entry *tags_entry;
struct tag *tag = (struct tag *)atags_copy;
- struct buffer *b;
+ struct buffer *b = NULL;
size_t size;
if (tag->hdr.tag != ATAG_CORE) {
@@ -54,13 +54,9 @@ static int __init init_atags_procfs(void)
WARN_ON(tag->hdr.tag != ATAG_NONE);
- b = kmalloc(sizeof(*b) + size, GFP_KERNEL);
- if (!b)
+ if (mem_to_flex_dup(&b, atags_copy, size, GFP_KERNEL))
goto nomem;
- b->size = size;
- memcpy(b->data, atags_copy, size);
-
tags_entry = proc_create_data("atags", 0400, NULL, &atags_proc_ops, b);
if (!tags_entry)
goto nomem;
--
2.32.0
[PATCH 21/32] soc: qcom: apr: Use mem_to_flex_dup() with struct apr_rx_buf
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Andy Gross
Cc: Bjorn Andersson
Cc: linux-arm-...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/soc/qcom/apr.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
index 3caabd873322..6cf6f6df276e 100644
--- a/drivers/soc/qcom/apr.c
+++ b/drivers/soc/qcom/apr.c
@@ -40,8 +40,8 @@ struct packet_router {
struct apr_rx_buf {
struct list_head node;
- int len;
- uint8_t buf[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(uint8_t, buf);
};
/**
@@ -162,7 +162,7 @@ static int apr_callback(struct rpmsg_device *rpdev, void
*buf,
int len, void *priv, u32 addr)
{
struct packet_router *apr = dev_get_drvdata(&rpdev->dev);
- struct apr_rx_buf *abuf;
+ struct apr_rx_buf *abuf = NULL;
unsigned long flags;
if (len <= APR_HDR_SIZE) {
@@ -171,13 +171,9 @@ static int apr_callback(struct rpmsg_device *rpdev, void
*buf,
return -EINVAL;
}
- abuf = kzalloc(sizeof(*abuf) + len, GFP_ATOMIC);
- if (!abuf)
+ if (mem_to_flex_dup(&abuf, buf, len, GFP_ATOMIC))
return -ENOMEM;
- abuf->len = len;
- memcpy(abuf->buf, buf, len);
-
spin_lock_irqsave(&apr->rx_lock, flags);
list_add_tail(&abuf->node, &apr->rx_list);
spin_unlock_irqrestore(&apr->rx_lock, flags);
--
2.32.0
[PATCH 29/32] xtensa: Use mem_to_flex_dup() with struct property
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Chris Zankel
Cc: Max Filippov
Cc: Rob Herring
Cc: Frank Rowand
Cc: Guenter Roeck
Cc: linux-xte...@linux-xtensa.org
Cc: devicet...@vger.kernel.org
Signed-off-by: Kees Cook
---
arch/xtensa/platforms/xtfpga/setup.c | 9 +++--
include/linux/of.h | 3 ++-
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/xtensa/platforms/xtfpga/setup.c
b/arch/xtensa/platforms/xtfpga/setup.c
index 538e6748e85a..31c1fa4ba4ec 100644
--- a/arch/xtensa/platforms/xtfpga/setup.c
+++ b/arch/xtensa/platforms/xtfpga/setup.c
@@ -102,7 +102,7 @@ CLK_OF_DECLARE(xtfpga_clk, "cdns,xtfpga-clock",
xtfpga_clk_setup);
#define MAC_LEN 6
static void __init update_local_mac(struct device_node *node)
{
- struct property *newmac;
+ struct property *newmac = NULL;
const u8* macaddr;
int prop_len;
@@ -110,19 +110,16 @@ static void __init update_local_mac(struct device_node
*node)
if (macaddr == NULL || prop_len != MAC_LEN)
return;
- newmac = kzalloc(sizeof(*newmac) + MAC_LEN, GFP_KERNEL);
- if (newmac == NULL)
+ if (mem_to_flex_dup(&newmac, macaddr, MAC_LEN, GFP_KERNEL))
return;
- newmac->value = newmac + 1;
- newmac->length = MAC_LEN;
+ newmac->value = newmac->contents;
newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
if (newmac->name == NULL) {
kfree(newmac);
return;
}
- memcpy(newmac->value, macaddr, MAC_LEN);
((u8*)newmac->value)[5] = (*(u32*)DIP_SWITCHES_VADDR) & 0x3f;
of_update_property(node, newmac);
}
diff --git a/include/linux/of.h b/include/linux/of.h
index 17741eee0ca4..efb0f419fd1f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -30,7 +30,7 @@ typedef u32 ihandle;
struct property {
char*name;
- int length;
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, length);
void*value;
struct property *next;
#if defined(CONFIG_OF_DYNAMIC) || defined(CONFIG_SPARC)
@@ -42,6 +42,7 @@ struct property {
#if defined(CONFIG_OF_KOBJ)
struct bin_attribute attr;
#endif
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, contents);
};
#if defined(CONFIG_SPARC)
--
2.32.0
[PATCH 15/32] 802/garp: Use mem_to_flex_dup() with struct garp_attr
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Hulk Robot
Cc: Yang Yingliang
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/net/garp.h | 4 ++--
net/802/garp.c | 9 +++--
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/include/net/garp.h b/include/net/garp.h
index 4d9a0c6a2e5f..ec087ae534e7 100644
--- a/include/net/garp.h
+++ b/include/net/garp.h
@@ -80,8 +80,8 @@ struct garp_attr {
struct rb_node node;
enum garp_applicant_state state;
u8 type;
- u8 dlen;
- unsigned char data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u8, dlen);
+ DECLARE_FLEX_ARRAY_ELEMENTS(unsigned char, data);
};
enum garp_applications {
diff --git a/net/802/garp.c b/net/802/garp.c
index f6012f8e59f0..72743ed00a54 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -168,7 +168,7 @@ static struct garp_attr *garp_attr_create(struct
garp_applicant *app,
const void *data, u8 len, u8 type)
{
struct rb_node *parent = NULL, **p = &app->gid.rb_node;
- struct garp_attr *attr;
+ struct garp_attr *attr = NULL;
int d;
while (*p) {
@@ -184,13 +184,10 @@ static struct garp_attr *garp_attr_create(struct
garp_applicant *app,
return attr;
}
}
- attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
- if (!attr)
- return attr;
+ if (mem_to_flex_dup(&attr, data, len, GFP_ATOMIC))
+ return NULL;
attr->state = GARP_APPLICANT_VO;
attr->type = type;
- attr->dlen = len;
- memcpy(attr->data, data, len);
rb_link_node(&attr->node, parent, p);
rb_insert_color(&attr->node, &app->gid);
--
2.32.0
[PATCH 08/32] iwlwifi: mvm: Use mem_to_flex_dup() with struct ieee80211_key_conf
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Luca Coelho
Cc: Kalle Valo
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Johannes Berg
Cc: Gregory Greenman
Cc: Eric Dumazet
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 ++--
include/net/mac80211.h | 4 ++--
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 406f0a50a5bf..23cade528dcf 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -4108,7 +4108,7 @@ int iwl_mvm_add_pasn_sta(struct iwl_mvm *mvm, struct
ieee80211_vif *vif,
int ret;
u16 queue;
struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
- struct ieee80211_key_conf *keyconf;
+ struct ieee80211_key_conf *keyconf = NULL;
ret = iwl_mvm_allocate_int_sta(mvm, sta, 0,
NL80211_IFTYPE_UNSPECIFIED,
@@ -4122,15 +4122,11 @@ int iwl_mvm_add_pasn_sta(struct iwl_mvm *mvm, struct
ieee80211_vif *vif,
if (ret)
goto out;
- keyconf = kzalloc(sizeof(*keyconf) + key_len, GFP_KERNEL);
- if (!keyconf) {
+ if (mem_to_flex_dup(&keyconf, key, key_len, GFP_KERNEL)) {
ret = -ENOBUFS;
goto out;
}
-
keyconf->cipher = cipher;
- memcpy(keyconf->key, key, key_len);
- keyconf->keylen = key_len;
ret = iwl_mvm_send_sta_key(mvm, sta->sta_id, keyconf, false,
0, NULL, 0, 0, true);
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 75880fc70700..4abe52963a96 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1890,8 +1890,8 @@ struct ieee80211_key_conf {
u8 hw_key_idx;
s8 keyidx;
u16 flags;
- u8 keylen;
- u8 key[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u8, keylen);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, key);
};
#define IEEE80211_MAX_PN_LEN 16
--
2.32.0
Re: [PATCH 01/32] netlink: Avoid memcpy() across flexible array boundary
On Tue, May 03, 2022 at 10:31:05PM -0500, Gustavo A. R. Silva wrote:
> On Tue, May 03, 2022 at 06:44:10PM -0700, Kees Cook wrote:
> [...]
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 1b5a9c2e1c29..09346aee1022 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct
> > nlmsghdr *nlh, int err,
> > NLMSG_ERROR, payload, flags);
> > errmsg = nlmsg_data(rep);
> > errmsg->error = err;
> > - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len :
> > sizeof(*nlh));
> > + errmsg->msg = *nlh;
> > + if (payload > sizeof(*errmsg))
> > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload,
> > + nlh->nlmsg_len - sizeof(*nlh));
>
> They have nlmsg_len()[1] for the length of the payload without the header:
>
> /**
> * nlmsg_len - length of message payload
> * @nlh: netlink message header
> */
> static inline int nlmsg_len(const struct nlmsghdr *nlh)
> {
> return nlh->nlmsg_len - NLMSG_HDRLEN;
> }
Oh, hm, yeah, that would be much cleaner. The relationship between
"payload" and nlmsg_len is confusing in here. :)
So, this should be simpler:
- memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len :
sizeof(*nlh));
+ errmsg->msg = *nlh;
+ memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh));
It's actually this case that triggered my investigation in __bos(1)'s
misbehavior around sub-structs, since this case wasn't getting silenced:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
It still feels like it should be possible to get this right without
splitting the memcpy, though. Hmpf.
> (would that function use some sanitization, though? what if nlmsg_len is
> somehow manipulated to be less than NLMSG_HDRLEN?...)
Maybe something like:
static inline int nlmsg_len(const struct nlmsghdr *nlh)
{
if (WARN_ON(nlh->nlmsg_len < NLMSG_HDRLEN))
return 0;
return nlh->nlmsg_len - NLMSG_HDRLEN;
}
> Also, it seems there is at least one more instance of this same issue:
>
> diff --git a/net/netfilter/ipset/ip_set_core.c
> b/net/netfilter/ipset/ip_set_core.c
> index 16ae92054baa..d06184b94af5 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -1723,7 +1723,8 @@ call_ad(struct net *net, struct sock *ctnl, struct
> sk_buff *skb,
> nlh->nlmsg_seq, NLMSG_ERROR, payload, 0);
> errmsg = nlmsg_data(rep);
> errmsg->error = ret;
> - memcpy(&errmsg->msg, nlh, nlh->nlmsg_len);
> + errmsg->msg = *nlh;
> + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload,
> nlmsg_len(nlh));
Ah, yes, nice catch!
> cmdattr = (void *)&errmsg->msg + min_len;
>
> ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr,
>
> --
> Gustavo
>
> [1]
> https://elixir.bootlin.com/linux/v5.18-rc5/source/include/net/netlink.h#L577
--
Kees Cook
[PATCH 05/32] brcmfmac: Use mem_to_flex_dup() with struct brcmf_fweh_queue_item
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Arend van Spriel
Cc: Franky Lin
Cc: Hante Meuleman
Cc: Kalle Valo
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: linux-wirel...@vger.kernel.org
Cc: brcm80211-dev-list@broadcom.com
Cc: sha-cyfmac-dev-l...@infineon.com
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
.../net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index bc3f4e4edcdf..bea798ca6466 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -32,8 +32,8 @@ struct brcmf_fweh_queue_item {
u8 ifidx;
u8 ifaddr[ETH_ALEN];
struct brcmf_event_msg_be emsg;
- u32 datalen;
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, datalen);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
/*
@@ -395,7 +395,7 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
{
enum brcmf_fweh_event_code code;
struct brcmf_fweh_info *fweh = &drvr->fweh;
- struct brcmf_fweh_queue_item *event;
+ struct brcmf_fweh_queue_item *event = NULL;
void *data;
u32 datalen;
@@ -414,8 +414,7 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
datalen + sizeof(*event_packet) > packet_len)
return;
- event = kzalloc(sizeof(*event) + datalen, gfp);
- if (!event)
+ if (mem_to_flex_dup(&event, data, datalen, gfp))
return;
event->code = code;
@@ -423,8 +422,6 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
/* use memcpy to get aligned event message */
memcpy(&event->emsg, &event_packet->msg, sizeof(event->emsg));
- memcpy(event->data, data, datalen);
- event->datalen = datalen;
memcpy(event->ifaddr, event_packet->eth.h_dest, ETH_ALEN);
brcmf_fweh_queue_event(fweh, event);
--
2.32.0
[PATCH 19/32] afs: Use mem_to_flex_dup() with struct afs_acl
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: David Howells
Cc: Marc Dionne
Cc: linux-...@lists.infradead.org
Signed-off-by: Kees Cook
---
fs/afs/internal.h | 4 ++--
fs/afs/xattr.c| 7 ++-
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 7a72e9c60423..83014d20b6b3 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -1125,8 +1125,8 @@ extern bool afs_fs_get_capabilities(struct afs_net *,
struct afs_server *,
extern void afs_fs_inline_bulk_status(struct afs_operation *);
struct afs_acl {
- u32 size;
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
extern void afs_fs_fetch_acl(struct afs_operation *);
diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
index 7751b0b3f81d..77b3af283d49 100644
--- a/fs/afs/xattr.c
+++ b/fs/afs/xattr.c
@@ -73,16 +73,13 @@ static int afs_xattr_get_acl(const struct xattr_handler
*handler,
static bool afs_make_acl(struct afs_operation *op,
const void *buffer, size_t size)
{
- struct afs_acl *acl;
+ struct afs_acl *acl = NULL;
- acl = kmalloc(sizeof(*acl) + size, GFP_KERNEL);
- if (!acl) {
+ if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL)) {
afs_op_nomem(op);
return false;
}
- acl->size = size;
- memcpy(acl->data, buffer, size);
op->acl = acl;
return true;
}
--
2.32.0
[PATCH 13/32] mac80211: Use mem_to_flex_dup() with several structs
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying:
struct probe_resp
struct fils_discovery_data
struct unsol_bcast_probe_resp_data
Cc: Johannes Berg
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
net/mac80211/cfg.c | 22 ++
net/mac80211/ieee80211_i.h | 12 ++--
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f1d211e61e49..355edbf41707 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -867,20 +867,16 @@ ieee80211_set_probe_resp(struct ieee80211_sub_if_data
*sdata,
const struct ieee80211_csa_settings *csa,
const struct ieee80211_color_change_settings *cca)
{
- struct probe_resp *new, *old;
+ struct probe_resp *new = NULL, *old;
if (!resp || !resp_len)
return 1;
old = sdata_dereference(sdata->u.ap.probe_resp, sdata);
- new = kzalloc(sizeof(struct probe_resp) + resp_len, GFP_KERNEL);
- if (!new)
+ if (mem_to_flex_dup(&new, resp, resp_len, GFP_KERNEL))
return -ENOMEM;
- new->len = resp_len;
- memcpy(new->data, resp, resp_len);
-
if (csa)
memcpy(new->cntdwn_counter_offsets, csa->counter_offsets_presp,
csa->n_counter_offsets_presp *
@@ -898,7 +894,7 @@ ieee80211_set_probe_resp(struct ieee80211_sub_if_data
*sdata,
static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
struct cfg80211_fils_discovery *params)
{
- struct fils_discovery_data *new, *old = NULL;
+ struct fils_discovery_data *new = NULL, *old = NULL;
struct ieee80211_fils_discovery *fd;
if (!params->tmpl || !params->tmpl_len)
@@ -909,11 +905,8 @@ static int ieee80211_set_fils_discovery(struct
ieee80211_sub_if_data *sdata,
fd->max_interval = params->max_interval;
old = sdata_dereference(sdata->u.ap.fils_discovery, sdata);
- new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
- if (!new)
+ if (mem_to_flex_dup(&new, params->tmpl, params->tmpl_len, GFP_KERNEL))
return -ENOMEM;
- new->len = params->tmpl_len;
- memcpy(new->data, params->tmpl, params->tmpl_len);
rcu_assign_pointer(sdata->u.ap.fils_discovery, new);
if (old)
@@ -926,17 +919,14 @@ static int
ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
struct cfg80211_unsol_bcast_probe_resp
*params)
{
- struct unsol_bcast_probe_resp_data *new, *old = NULL;
+ struct unsol_bcast_probe_resp_data *new = NULL, *old = NULL;
if (!params->tmpl || !params->tmpl_len)
return -EINVAL;
old = sdata_dereference(sdata->u.ap.unsol_bcast_probe_resp, sdata);
- new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
- if (!new)
+ if (mem_to_flex_dup(&new, params->tmpl, params->tmpl_len, GFP_KERNEL))
return -ENOMEM;
- new->len = params->tmpl_len;
- memcpy(new->data, params->tmpl, params->tmpl_len);
rcu_assign_pointer(sdata->u.ap.unsol_bcast_probe_resp, new);
if (old)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d4a7ba4a8202..2e9bbfb12c0d 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -263,21 +263,21 @@ struct beacon_data {
struct probe_resp {
struct rcu_head rcu_head;
- int len;
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len);
u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM];
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
struct fils_discovery_data {
struct rcu_head rcu_head;
- int len;
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
struct unsol_bcast_probe_resp_data {
struct rcu_head rcu_head;
- int len;
- u8 data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
};
struct ps_data {
--
2.32.0
[PATCH 17/32] net/flow_offload: Use mem_to_flex_dup() with struct flow_action_cookie
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: Baowen Zheng
Cc: Eli Cohen
Cc: Louis Peens
Cc: Simon Horman
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/net/flow_offload.h | 4 ++--
net/core/flow_offload.c| 7 ++-
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 021778a7e1af..ca5db457a0bc 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -190,8 +190,8 @@ enum flow_action_hw_stats {
typedef void (*action_destr)(void *priv);
struct flow_action_cookie {
- u32 cookie_len;
- u8 cookie[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, cookie_len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, cookie);
};
struct flow_action_cookie *flow_action_cookie_create(void *data,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 73f68d4625f3..e23c8d05b828 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -199,13 +199,10 @@ struct flow_action_cookie *flow_action_cookie_create(void
*data,
unsigned int len,
gfp_t gfp)
{
- struct flow_action_cookie *cookie;
+ struct flow_action_cookie *cookie = NULL;
- cookie = kmalloc(sizeof(*cookie) + len, gfp);
- if (!cookie)
+ if (mem_to_flex_dup(&cookie, data, len, gfp))
return NULL;
- cookie->cookie_len = len;
- memcpy(cookie->cookie, data, len);
return cookie;
}
EXPORT_SYMBOL(flow_action_cookie_create);
--
2.32.0
[PATCH 24/32] IB/hfi1: Use mem_to_flex_dup() for struct tid_rb_node
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Dennis Dalessandro
Cc: Jason Gunthorpe
Cc: Leon Romanovsky
Cc: linux-r...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/infiniband/hw/hfi1/user_exp_rcv.c | 7 ++-
drivers/infiniband/hw/hfi1/user_exp_rcv.h | 4 ++--
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 186d30291260..f14846662ac9 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -683,7 +683,7 @@ static int set_rcvarray_entry(struct hfi1_filedata *fd,
{
int ret;
struct hfi1_ctxtdata *uctxt = fd->uctxt;
- struct tid_rb_node *node;
+ struct tid_rb_node *node = NULL;
struct hfi1_devdata *dd = uctxt->dd;
dma_addr_t phys;
struct page **pages = tbuf->pages + pageidx;
@@ -692,8 +692,7 @@ static int set_rcvarray_entry(struct hfi1_filedata *fd,
* Allocate the node first so we can handle a potential
* failure before we've programmed anything.
*/
- node = kzalloc(struct_size(node, pages, npages), GFP_KERNEL);
- if (!node)
+ if (mem_to_flex_dup(&node, pages, npages, GFP_KERNEL))
return -ENOMEM;
phys = dma_map_single(&dd->pcidev->dev, __va(page_to_phys(pages[0])),
@@ -707,12 +706,10 @@ static int set_rcvarray_entry(struct hfi1_filedata *fd,
node->fdata = fd;
node->phys = page_to_phys(pages[0]);
- node->npages = npages;
node->rcventry = rcventry;
node->dma_addr = phys;
node->grp = grp;
node->freed = false;
- memcpy(node->pages, pages, flex_array_size(node, pages, npages));
if (fd->use_mn) {
ret = mmu_interval_notifier_insert(
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.h
b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
index 8c53e416bf84..4be3446c4d25 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.h
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
@@ -32,8 +32,8 @@ struct tid_rb_node {
u32 rcventry;
dma_addr_t dma_addr;
bool freed;
- unsigned int npages;
- struct page *pages[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned int, npages);
+ DECLARE_FLEX_ARRAY_ELEMENTS(struct page *, pages);
};
static inline int num_user_pages(unsigned long addr,
--
2.32.0
[PATCH 31/32] xenbus: Use mem_to_flex_dup() with struct read_buffer
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Kees Cook
---
drivers/xen/xenbus/xenbus_dev_frontend.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c
b/drivers/xen/xenbus/xenbus_dev_frontend.c
index 597af455a522..4267aaef33fb 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -81,8 +81,8 @@ struct xenbus_transaction_holder {
struct read_buffer {
struct list_head list;
unsigned int cons;
- unsigned int len;
- char msg[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned int, len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(char, msg);
};
struct xenbus_file_priv {
@@ -188,21 +188,17 @@ static ssize_t xenbus_file_read(struct file *filp,
*/
static int queue_reply(struct list_head *queue, const void *data, size_t len)
{
- struct read_buffer *rb;
+ struct read_buffer *rb = NULL;
if (len == 0)
return 0;
if (len > XENSTORE_PAYLOAD_MAX)
return -EINVAL;
- rb = kmalloc(sizeof(*rb) + len, GFP_KERNEL);
- if (rb == NULL)
+ if (mem_to_flex_dup(&rb, data, len, GFP_KERNEL))
return -ENOMEM;
rb->cons = 0;
- rb->len = len;
-
- memcpy(rb->msg, data, len);
list_add_tail(&rb->list, queue);
return 0;
--
2.32.0
[PATCH 26/32] ima: Use mem_to_flex_dup() with struct modsig
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Mimi Zohar
Cc: Dmitry Kasatkin
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: linux-integr...@vger.kernel.org
Cc: linux-security-mod...@vger.kernel.org
Signed-off-by: Kees Cook
---
security/integrity/ima/ima_modsig.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
index fb25723c65bc..200c080d36de 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -28,8 +28,8 @@ struct modsig {
* This is what will go to the measurement list if the template requires
* storing the signature.
*/
- int raw_pkcs7_len;
- u8 raw_pkcs7[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, raw_pkcs7_len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, raw_pkcs7);
};
/*
@@ -42,7 +42,7 @@ int ima_read_modsig(enum ima_hooks func, const void *buf,
loff_t buf_len,
{
const size_t marker_len = strlen(MODULE_SIG_STRING);
const struct module_signature *sig;
- struct modsig *hdr;
+ struct modsig *hdr = NULL;
size_t sig_len;
const void *p;
int rc;
@@ -65,8 +65,7 @@ int ima_read_modsig(enum ima_hooks func, const void *buf,
loff_t buf_len,
buf_len -= sig_len + sizeof(*sig);
/* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
- hdr = kzalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
- if (!hdr)
+ if (mem_to_flex_dup(&hdr, buf + buf_len, sig_len, GFP_KERNEL))
return -ENOMEM;
hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
@@ -76,9 +75,6 @@ int ima_read_modsig(enum ima_hooks func, const void *buf,
loff_t buf_len,
return rc;
}
- memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
- hdr->raw_pkcs7_len = sig_len;
-
/* We don't know the hash algorithm yet. */
hdr->hash_algo = HASH_ALGO__LAST;
--
2.32.0
[PATCH 30/32] usb: gadget: f_fs: Use mem_to_flex_dup() with struct ffs_buffer
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Felipe Balbi
Cc: Greg Kroah-Hartman
Cc: Eugeniu Rosca
Cc: John Keeping
Cc: Jens Axboe
Cc: Udipto Goswami
Cc: Andrew Gabbasov
Cc: linux-...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/usb/gadget/function/f_fs.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 4585ee3a444a..bb0ff41dabd2 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -202,9 +202,9 @@ struct ffs_epfile {
};
struct ffs_buffer {
- size_t length;
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, length);
char *data;
- char storage[];
+ DECLARE_FLEX_ARRAY_ELEMENTS(char, storage);
};
/* ffs_io_data structure ***/
@@ -905,7 +905,7 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile
*epfile,
void *data, int data_len,
struct iov_iter *iter)
{
- struct ffs_buffer *buf;
+ struct ffs_buffer *buf = NULL;
ssize_t ret = copy_to_iter(data, data_len, iter);
if (data_len == ret)
@@ -919,12 +919,9 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile
*epfile,
data_len, ret);
data_len -= ret;
- buf = kmalloc(struct_size(buf, storage, data_len), GFP_KERNEL);
- if (!buf)
+ if (mem_to_flex_dup(&buf, data + ret, data_len, GFP_KERNEL))
return -ENOMEM;
- buf->length = data_len;
buf->data = buf->storage;
- memcpy(buf->storage, data + ret, flex_array_size(buf, storage,
data_len));
/*
* At this point read_buffer is NULL or READ_BUFFER_DROP (if
--
2.32.0
[PATCH 32/32] esas2r: Use __mem_to_flex() with struct atto_ioctl
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying. This requires adding the
flexible array explicitly.
Cc: Bradley Grove
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-s...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/esas2r/atioctl.h | 1 +
drivers/scsi/esas2r/esas2r_ioctl.c | 11 +++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/esas2r/atioctl.h b/drivers/scsi/esas2r/atioctl.h
index ff2ad9b38575..dd3437412ffc 100644
--- a/drivers/scsi/esas2r/atioctl.h
+++ b/drivers/scsi/esas2r/atioctl.h
@@ -831,6 +831,7 @@ struct __packed atto_hba_trace {
u32 total_length;
u32 trace_mask;
u8 reserved2[48];
+ u8 contents[];
};
#define ATTO_FUNC_SCSI_PASS_THRU 0x04
diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c
b/drivers/scsi/esas2r/esas2r_ioctl.c
index 08f4e43c7d9e..9310b54b1575 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
@@ -947,11 +947,14 @@ static int hba_ioctl_callback(struct esas2r_adapter *a,
break;
}
- memcpy(trc + 1,
- a->fw_coredump_buff + offset,
- len);
+ if (__mem_to_flex(hi, data.trace.contents,
+ data_length,
+ a->fw_coredump_buff + offset,
+ len)) {
+ hi->status = ATTO_STS_INV_FUNC;
+ break;
+ }
- hi->data_length = len;
} else if (trc->trace_func == ATTO_TRC_TF_RESET) {
memset(a->fw_coredump_buff, 0,
ESAS2R_FWCOREDUMP_SZ);
--
2.32.0
[PATCH 25/32] Drivers: hv: utils: Use mem_to_flex_dup() with struct cn_msg
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: Stephen Hemminger
Cc: Wei Liu
Cc: Dexuan Cui
Cc: linux-hyp...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/hv/hv_utils_transport.c | 7 ++-
include/uapi/linux/connector.h | 4 ++--
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 832885198643..43b4f8893cc0 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -217,20 +217,17 @@ static void hvt_cn_callback(struct cn_msg *msg, struct
netlink_skb_parms *nsp)
int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len,
void (*on_read_cb)(void))
{
- struct cn_msg *cn_msg;
+ struct cn_msg *cn_msg = NULL;
int ret = 0;
if (hvt->mode == HVUTIL_TRANSPORT_INIT ||
hvt->mode == HVUTIL_TRANSPORT_DESTROY) {
return -EINVAL;
} else if (hvt->mode == HVUTIL_TRANSPORT_NETLINK) {
- cn_msg = kzalloc(sizeof(*cn_msg) + len, GFP_ATOMIC);
- if (!cn_msg)
+ if (mem_to_flex_dup(&cn_msg, msg, len, GFP_ATOMIC))
return -ENOMEM;
cn_msg->id.idx = hvt->cn_id.idx;
cn_msg->id.val = hvt->cn_id.val;
- cn_msg->len = len;
- memcpy(cn_msg->data, msg, len);
ret = cn_netlink_send(cn_msg, 0, 0, GFP_ATOMIC);
kfree(cn_msg);
/*
diff --git a/include/uapi/linux/connector.h b/include/uapi/linux/connector.h
index 3738936149a2..b85bbe753dae 100644
--- a/include/uapi/linux/connector.h
+++ b/include/uapi/linux/connector.h
@@ -73,9 +73,9 @@ struct cn_msg {
__u32 seq;
__u32 ack;
- __u16 len; /* Length of the following data */
+ __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, len);
__u16 flags;
- __u8 data[0];
+ __DECLARE_FLEX_ARRAY_ELEMENTS(__u8, data);
};
#endif /* _UAPI__CONNECTOR_H */
--
2.32.0
[PATCH 23/32] Bluetooth: Use mem_to_flex_dup() with struct hci_op_configure_data_path
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.
Cc: Marcel Holtmann
Cc: Johan Hedberg
Cc: Luiz Augusto von Dentz
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: linux-blueto...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/net/bluetooth/hci.h | 4 ++--
net/bluetooth/hci_request.c | 9 ++---
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 62a9bb022aed..7b398ef0b46d 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1321,8 +1321,8 @@ struct hci_rp_read_local_oob_ext_data {
struct hci_op_configure_data_path {
__u8direction;
__u8data_path_id;
- __u8vnd_len;
- __u8vnd_data[];
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u8, vnd_len);
+ DECLARE_FLEX_ARRAY_ELEMENTS(__u8, vnd_data);
} __packed;
#define HCI_OP_READ_LOCAL_VERSION 0x1001
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index f4afe482e300..e29be3810b93 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -2435,19 +2435,14 @@ int hci_req_configure_datapath(struct hci_dev *hdev,
struct bt_codec *codec)
if (err < 0)
goto error;
- cmd = kzalloc(sizeof(*cmd) + vnd_len, GFP_KERNEL);
- if (!cmd) {
- err = -ENOMEM;
+ err = mem_to_flex_dup(&cmd, vnd_data, vnd_len, GFP_KERNEL);
+ if (err < 0)
goto error;
- }
err = hdev->get_data_path_id(hdev, &cmd->data_path_id);
if (err < 0)
goto error;
- cmd->vnd_len = vnd_len;
- memcpy(cmd->vnd_data, vnd_data, vnd_len);
-
cmd->direction = 0x00;
hci_req_add(&req, HCI_CONFIGURE_DATA_PATH, sizeof(*cmd) + vnd_len, cmd);
--
2.32.0
Re: [PATCH 10/32] wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg
On Wed, May 04, 2022 at 08:42:46AM +0300, Kalle Valo wrote:
> Kees Cook writes:
>
> > As part of the work to perform bounds checking on all memcpy() uses,
> > replace the open-coded a deserialization of bytes out of memory into a
> > trailing flexible array by using a flex_array.h helper to perform the
> > allocation, bounds checking, and copying.
> >
> > Cc: Loic Poulain
> > Cc: Kalle Valo
> > Cc: "David S. Miller"
> > Cc: Eric Dumazet
> > Cc: Jakub Kicinski
> > Cc: Paolo Abeni
> > Cc: wcn3...@lists.infradead.org
> > Cc: linux-wirel...@vger.kernel.org
> > Cc: net...@vger.kernel.org
> > Signed-off-by: Kees Cook
>
> [...]
>
> > --- a/drivers/net/wireless/ath/wcn36xx/smd.h
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.h
> > @@ -46,8 +46,8 @@ struct wcn36xx_fw_msg_status_rsp {
> >
> > struct wcn36xx_hal_ind_msg {
> > struct list_head list;
> > - size_t msg_len;
> > - u8 msg[];
> > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, msg_len);
> > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, msg);
>
> This affects readability quite a lot and tbh I don't like it. Isn't
> there any simpler way to solve this?
Similar to how I plumbed member names into __mem_to_flex(), I could do
the same for __mem_to_flex_dup(). That way if the struct member aliases
(DECLARE_FLEX...) aren't added, the longer form of the helper could
be used. Instead of:
if (mem_to_flex_dup(&msg_ind, buf, len, GFP_ATOMIC)) {
it would be:
if (__mem_to_flex_dup(&msg_ind, /* self */, msg,
msg_len, buf, len, GFP_ATOMIC)) {
This was how I'd written the helpers in an earlier version, but it
seemed much cleaner to avoid repeating structure layout details at each
call site.
I couldn't find any other way to encode the needed information. It'd be
wonderful if C would let us do:
struct wcn36xx_hal_ind_msg {
struct list_head list;
size_t msg_len;
u8 msg[msg_len];
}
And provide some kind of interrogation:
__builtin_flex_array_member(msg_ind) -> msg_ind->msg
__builtin_flex_array_count(msg_ind) -> msg_ind->msg_len
My hope would be to actually use the member aliases to teach things like
-fsanitize=array-bounds about flexible arrays. If it encounters a
structure with the aliases, it could add the instrumentation to do the
bounds checking of things like:
msg_ind->msg[42]; /* check that 42 is < msg_ind->msg_len */
I also wish I could find a way to make the proposed macros "forward
portable" into proposed C syntax above, but this eluded me as well.
For example:
struct wcn36xx_hal_ind_msg {
size_t msg_len;
struct list_head list;
BOUNDED_FLEX_ARRAY(u8, msg, msg_len);
}
#ifdef CC_HAS_DYNAMIC_ARRAY_LEN
# define BOUNDED_FLEX_ARRAY(type, name, bounds) type name[bounds]
#else
# define BOUNDED_FLEX_ARRAY(type, name, bounds) \
magic_alias_of msg_len __flex_array_elements_count; \
union { \
type name[];\
type __flex_array_elements[]; \
}
#endif
But I couldn't sort out the "magic_alias_of" syntax that wouldn't force
structures into having the count member immediately before the flex
array, which would impose more limitations on where this could be
used...
Anyway, I'm open to ideas on how to improve this!
-Kees
--
Kees Cook
Re: [PATCH 12/32] cfg80211: Use mem_to_flex_dup() with struct cfg80211_bss_ies
On Wed, May 04, 2022 at 09:28:46AM +0200, Johannes Berg wrote: > On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > > > @@ -2277,7 +2274,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy > > *wiphy, > > size_t ielen = len - offsetof(struct ieee80211_mgmt, > > u.probe_resp.variable); > > size_t new_ie_len; > > - struct cfg80211_bss_ies *new_ies; > > + struct cfg80211_bss_ies *new_ies = NULL; > > const struct cfg80211_bss_ies *old; > > u8 cpy_len; > > > > @@ -2314,8 +2311,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy > > *wiphy, > > if (!new_ie) > > return; > > > > - new_ies = kzalloc(sizeof(*new_ies) + new_ie_len, GFP_ATOMIC); > > - if (!new_ies) > > + if (mem_to_flex_dup(&new_ies, new_ie, new_ie_len, GFP_ATOMIC)) > > goto out_free; > > > > pos = new_ie; > > @@ -2333,10 +2329,8 @@ cfg80211_update_notlisted_nontrans(struct wiphy > > *wiphy, > > memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len))); > > > > /* update ie */ > > - new_ies->len = new_ie_len; > > new_ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp); > > new_ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control); > > - memcpy(new_ies->data, new_ie, new_ie_len); > > This introduces a bug, "new_ie" is modified between the kzalloc() and > the memcpy(), but you've moved the memcpy() into the allocation. In > fact, new_ie is completely freshly kzalloc()'ed at this point. So you > need to change the ordering here, but since new_ie is freed pretty much > immediately, we can probably just build the stuff directly inside > new_ies->data, though then of course we cannot use your helper anymore? Eek, yes, thanks. My attempt to locate the alloc/memcpy pattern failed to take into account anything touch the source between alloc and memcpy. I'll double check the other examples. -Kees -- Kees Cook
Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers
On Wed, May 04, 2022 at 09:25:56AM +0200, Johannes Berg wrote:
> On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote:
> >
> > For example, using the most complicated helper, mem_to_flex_dup():
> >
> > /* Flexible array struct with members identified. */
> > struct something {
> > int mode;
> > DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, how_many);
> > unsigned long flags;
> > DECLARE_FLEX_ARRAY_ELEMENTS(u32, value);
>
> In many cases, the order of the elements doesn't really matter, so maybe
> it'd be nicer to be able to write it as something like
>
> DECLARE_FLEX_STRUCT(something,
> int mode;
> unsigned long flags;
> ,
> int, how_many,
> u32, value);
>
> perhaps? OK, that doesn't seem so nice either.
>
> Maybe
>
> struct something {
> int mode;
> unsigned long flags;
> FLEX_ARRAY(
> int, how_many,
> u32, value
> );
> };
Yeah, I mention some of my exploration of this idea in the sibling reply:
https://lore.kernel.org/linux-hardening/202205040730.161645EC@keescook/#t
It seemed like requiring a structure be rearranged to take advantage of
the "automatic layout introspection" wasn't very friendly. On the other
hand, looking at the examples, most of them are already neighboring
members. Hmmm.
> or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and
> DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases
> where the struct layout is not the most important thing (or it's already
> at the end anyway).
The names aren't great, but I wanted to distinguish "elements" as the
array not the count. Yay naming.
However, perhaps the solution is to have _both_. i.e using
BOUNDED_FLEX_ARRAY(count_type, count_name, array_type, array_name) for
the "neighboring" case, and the DECLARE...{ELEMENTS,COUNT} for the
"split" case.
And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include
the count_name too, so both methods could be "forward portable" to a
future where C grew the syntax for bounded flex arrays.
>
> > struct something *instance = NULL;
> > int rc;
> >
> > rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL);
> > if (rc)
> > return rc;
>
> This seems rather awkward, having to set it to NULL, then checking rc
> (and possibly needing a separate variable for it), etc.
I think the errno return is completely required. I had an earlier version
of this that was much more like a drop-in replacement for memcpy that
would just truncate or panic, and when I had it all together, I could
just imagine hearing Linus telling me to start over because it was unsafe
(truncation may be just as bad as overflow) and disruptive ("never BUG"),
and that it should be recoverable. So, I rewrote it all to return a
__must_check errno.
Requiring instance to be NULL is debatable, but I feel pretty strongly
about it because it does handle a class of mistakes (resource leaks),
and it's not much of a burden to require a known-good starting state.
> But I can understand how you arrived at this:
> - need to pass instance or &instance or such for typeof()
>or offsetof() or such
Right.
> - instance = mem_to_flex_dup(instance, ...)
>looks too much like it would actually dup 'instance', rather than
>'byte_array'
And I need an errno output to keep imaginary Linus happy. :)
> - if you pass &instance anyway, checking for NULL is simple and adds a
>bit of safety
Right.
> but still, honestly, I don't like it. As APIs go, it feels a bit
> cumbersome and awkward to use, and you really need everyone to use this,
> and not say "uh what, I'll memcpy() instead".
Sure, and I have tried to get it down as small as possible. The earlier
"just put all the member names in every call" version was horrid. :P I
realize it's more work to check errno, but the memcpy() API we've all
been trained to use is just plain dangerous. I don't think it's
unreasonable to ask people to retrain themselves to avoid it. All that
said, yes, I want it to be as friendly as possible.
> Maybe there should also be a realloc() version of it?
Sure! Seems reasonable. I'd like to see the code pattern for this
though. Do you have any examples? Most of what I'd been able to find for
the fragile memcpy() usage was just basic serialize/deserialize or
direct copying.
> > +/** __fas_bytes - Calculate potential size of flexible array structure
>
> I think you forgot "\n *" in many cases here after "/**".
Oops! Yes, thank you. I'll fix these.
-Kees
--
Kees Cook
Re: [PATCH 03/32] flex_array: Add Kunit tests
On Wed, May 04, 2022 at 11:00:38AM +0800, David Gow wrote:
> On Wed, May 4, 2022 at 9:47 AM Kees Cook wrote:
> >
> > Add tests for the new flexible array structure helpers. These can be run
> > with:
> >
> > make ARCH=um mrproper
> > ./tools/testing/kunit/kunit.py config
>
> Nit: it shouldn't be necessary to run kunit.py config separately:
> kunit.py run will configure the kernel if necessary.
Ah yes, I think you mentioned this before. I'll adjust the commit log.
>
> > ./tools/testing/kunit/kunit.py run flex_array
> >
> > Cc: David Gow
> > Cc: kunit-...@googlegroups.com
> > Signed-off-by: Kees Cook
> > ---
>
> This looks pretty good to me: it certainly worked on the different
> setups I tried (um, x86_64, x86_64+KASAN).
>
> A few minor nitpicks inline, mostly around minor config-y things, or
> things which weren't totally clear on my first read-through.
>
> Hopefully one day, with the various stubbing features or something
> similar, we'll be able to check against allocation failures in
> flex_dup(), too, but otherwise nothing seems too obviously missing.
>
> Reviewed-by: David Gow
Great; thanks for the review and testing!
>
> -- David
>
> > lib/Kconfig.debug | 12 +-
> > lib/Makefile | 1 +
> > lib/flex_array_kunit.c | 523 +
> > 3 files changed, 531 insertions(+), 5 deletions(-)
> > create mode 100644 lib/flex_array_kunit.c
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 9077bb38bc93..8bae6b169c50 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST
> > Builds unit tests for the check_*_overflow(), size_*(),
> > allocation, and
> > related functions.
> >
> > - For more information on KUnit and unit tests in general please
> > refer
> > - to the KUnit documentation in Documentation/dev-tools/kunit/.
> > -
> > - If unsure, say N.
> > -
>
> Nit: while I'm not against removing some of this boilerplate, is it
> better suited for a separate commit?
Make sense, yes. I'll drop this for now.
>
> > config STACKINIT_KUNIT_TEST
> > tristate "Test level of stack variable initialization" if
> > !KUNIT_ALL_TESTS
> > depends on KUNIT
> > @@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST
> > CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
> > or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
> >
> > +config FLEX_ARRAY_KUNIT_TEST
> > + tristate "Test flex_*() family of helper functions at runtime" if
> > !KUNIT_ALL_TESTS
> > + depends on KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + Builds unit tests for flexible array copy helper functions.
> > +
>
> Nit: checkpatch warns that the description here may be insufficient:
> WARNING: please write a help paragraph that fully describes the config symbol
Yeah, I don't know anything to put here that isn't just more
boilerplate, so I'm choosing to ignore this for now. :)
> > [...]
> > +struct normal {
> > + size_t datalen;
> > + u32 data[];
> > +};
> > +
> > +struct decl_normal {
> > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, datalen);
> > + DECLARE_FLEX_ARRAY_ELEMENTS(u32, data);
> > +};
> > +
> > +struct aligned {
> > + unsigned short datalen;
> > + chardata[] __aligned(__alignof__(u64));
> > +};
> > +
> > +struct decl_aligned {
> > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned short, datalen);
> > + DECLARE_FLEX_ARRAY_ELEMENTS(char, data) __aligned(__alignof__(u64));
> > +};
> > +
> > +static void struct_test(struct kunit *test)
> > +{
> > + COMPARE_STRUCTS(struct normal, struct decl_normal);
> > + COMPARE_STRUCTS(struct aligned, struct decl_aligned);
> > +}
>
> If I understand it, the purpose of this is to ensure that structs both
> with and without the flexible array declaration have the same memory
> layout?
>
> If so, any chance of a comment briefly stating that's the purpose (or
> renaming this test struct_layout_test())?
Yeah, good idea; I'll improve the naming.
>
> Also, would it make sense to do the same with the struct with internal
> padding below?
Heh, yes, good point! :)
> [...]
> > +#define CHECK_COPY(ptr)do {
Re: [PATCH 28/32] selinux: Use mem_to_flex_dup() with xfrm and sidtab
On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote:
> On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva
> wrote:
> >
> > Hi Paul,
> >
> > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> > > On Tue, May 3, 2022 at 9:57 PM Kees Cook wrote:
> >
> > [..]
> >
> > > > +++ b/include/uapi/linux/xfrm.h
> > > > @@ -31,9 +31,9 @@ struct xfrm_id {
> > > > struct xfrm_sec_ctx {
> > > > __u8ctx_doi;
> > > > __u8ctx_alg;
> > > > - __u16 ctx_len;
> > > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len);
> > > > __u32 ctx_sid;
> > > > - charctx_str[0];
> > > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str);
> > > > };
> > >
> > > While I like the idea of this in principle, I'd like to hear about the
> > > testing you've done on these patches. A previous flex array
> > > conversion in the audit uapi headers ended up causing a problem with
> >
> > I'm curious about which commit caused those problems...?
>
> Commit ed98ea2128b6 ("audit: replace zero-length array with
> flexible-array member"), however, as I said earlier, the problem was
> actually with SWIG, it just happened to be triggered by the kernel
> commit. There was a brief fedora-devel mail thread about the problem,
> see the link below:
>
> * https://www.spinics.net/lists/fedora-devel/msg297991.html
Wow, that's pretty weird -- it looks like SWIG was scraping the headers
to build its conversions? I assume SWIG has been fixed now?
> To reiterate, I'm supportive of changes like this, but I would like to
> hear how it was tested to ensure there are no unexpected problems with
> userspace. If there are userspace problems it doesn't mean we can't
> make changes like this, it just means we need to ensure that the
> userspace issues are resolved first.
Well, as this is the first and only report of any problems with [0] -> []
conversions (in UAPI or anywhere) that I remember seeing, and they've
been underway since at least v5.9, I hadn't been doing any new testing.
So, for this case, I guess I should ask what tests you think would be
meaningful here? Anything using #include should be fine:
https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1
Which leaves just this, which may be doing something weird:
libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi
But I see that SWIG doesn't show up in a search for linux/audit.h:
https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1
So this may not be a sufficient analysis...
--
Kees Cook
Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers
On Thu, May 05, 2022 at 08:16:11AM -0700, Keith Packard wrote: > Johannes Berg writes: > > > Yeah, dunno, I guess I'm slightly more on the side of not requiring it, > > since we don't do the same for kmalloc() etc. and probably really > > wouldn't want to add kmalloc_s() that does it ;-) > > I suspect the number of bugs this catches will be small, but they'll be > in places where the flow of control is complicated. What we want is to > know that there's no "real" value already present. I'd love it if we > could make the macro declare a new name (yeah, I know, mixing > declarations and code). I don't think I can do a declaration and an expression statement at the same time with different scopes, but that would be kind of cool. We did just move to c11 to gain the in-loop iterator declarations... > Of course, we could also end up with people writing a wrapping macro > that sets the variable to NULL before invoking the underlying macro... I hope it won't come to that! :) -- Kees Cook
Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers
On Thu, May 05, 2022 at 03:16:19PM +0200, Johannes Berg wrote: > On Wed, 2022-05-04 at 08:38 -0700, Kees Cook wrote: > > > > It seemed like requiring a structure be rearranged to take advantage of > > the "automatic layout introspection" wasn't very friendly. On the other > > hand, looking at the examples, most of them are already neighboring > > members. Hmmm. > > A lot of them are, and many could be, though not all. Yeah, I did a pass through them for the coming v2. Only a few have the struct order as part of an apparent hardware interface. > > And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include > > the count_name too, so both methods could be "forward portable" to a > > future where C grew the syntax for bounded flex arrays. > > I guess I don't see that happening :) Well ... it's on my roadmap. ;) I want it for -fsanitize=array-bounds so that dynamic array indexing can be checked too. (Right now we can do constant-sized array index bounds checking at runtime, but the much harder to find problems tend to come from flex arrays.) > > Requiring instance to be NULL is debatable, but I feel pretty strongly > > about it because it does handle a class of mistakes (resource leaks), > > and it's not much of a burden to require a known-good starting state. > > Yeah, dunno, I guess I'm slightly more on the side of not requiring it, > since we don't do the same for kmalloc() etc. and probably really > wouldn't want to add kmalloc_s() that does it ;-) Well, I dislike all the *alloc APIs. :P > I mean, you _could_ go there: > > int kmalloc_s(void **ptr, size_t size, gfp_t gfp) Oh, and I really do (though as a macro, not a "real" function), since having type introspection would be _extremely_ useful. Though maybe it needs to be through some kind of type-of-lvalue thing... https://github.com/KSPP/linux/issues/189 https://github.com/KSPP/linux/issues/87 > So I'm not really sure why this aspect here should need to be different, > except of course that you already need the input argument for the magic. Right, and trying to move the kernel code closer to a form where the compiler can take more of the burden of handling code safety. > And btw, while I was writing it down I was looking to see if it should > be "size_t elements" or "size_t len" (like memcpy), it took me some time > to figure out, and I was looking at the examples: > > 1) most of them actually use __u8 or some variant thereof, so you > could probably add an even simpler macro like >BOUNDED_FLEX_DATA(int, bytes, data) > which has the u8 type internally. I didn't want these helpers to be "opinionated" about their types (just their API), so while it's true u8 is usually "good enough", I don't think it's common enough to make a special case for. > 2) Unless I'm confusing myself, you got the firewire change wrong, > because __mem_to_flex_dup takes the "elements_count", but the > memcpy() there wasn't multiplied by the sizeof(element)? Or maybe > the fact that it was declared as __u32 header[0] is wrong, and it > should be __u8, but it's all very confusing, and I'm really not > sure about this at all. Yes indeed; thanks for catching that. In fact, it's not a strict flex array struct, since, as you say, it's measuring bytes, not elements. Yeah, I'll see if that needs to be adjusted/dropped, etc. > One "perhaps you'll laugh me out of the room" suggestion might be to > actually be able to initialize the whole thing too? > > mydata = flex_struct_alloc(mydata, GFP_KERNEL, >variable_data, variable_len, >.member = 1, >.another = 2); > > (the ordering can't really be otherwise since you have to use > __VA_ARGS__). Oooh, that's a cool idea for the API. H. > That might reduce some more code too, though I guess it's quite some > additional magic ... :) Yay preprocessor magic! > I was going to point to struct cfg80211_bss_ies, but I realize now > they're RCU-managed, so we never resize them anyway ... So maybe it's > less common than I thought it might be. > > I suppose you know better since you converted a lot of stuff already :-) Well, I've seen a lot of fragile code (usually in the form of exploitable flaws around flex arrays) and they do mostly look the same. Not everything fits perfectly into the forms this API tries to address, but my goal is to get it fitting well enough, and the weird stuff can be more carefully examined -- they're easier to find and audit if all the others are nicely wrapped up in some fancy flex*() API. Thanks for your thoughts on all of this! I'll continue to work on a v2... -Kees -- Kees Cook
Re: [PATCH 19/32] afs: Use mem_to_flex_dup() with struct afs_acl
On Thu, May 12, 2022 at 10:41:05PM +0100, David Howells wrote:
>
> Kees Cook wrote:
>
> > struct afs_acl {
> > - u32 size;
> > - u8 data[];
> > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size);
> > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
> > };
>
> Oof... That's really quite unpleasant syntax. Is it not possible to have
> mem_to_flex_dup() and friends work without that? You are telling them the
> fields they have to fill in.
Other threads discussed this too. I'm hoping to have something more
flexible (pardon the pun) in v2.
> [...]
> or:
>
> ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL);
> if (ret < 0)
>
> (or use != 0 rather than < 0)
Sure, I can make the tests more explicit. The kerndoc, etc all shows it's
using < 0 for errors.
--
Kees Cook
[PATCH 80/82] xen-netback: Refactor intentional wrap-around test
In an effort to separate intentional arithmetic wrap-around from unexpected wrap-around, we need to refactor places that depend on this kind of math. One of the most common code patterns of this is: VAR + value < VAR Notably, this is considered "undefined behavior" for signed and pointer types, which the kernel works around by using the -fno-strict-overflow option in the build[1] (which used to just be -fwrapv). Regardless, we want to get the kernel source to the position where we can meaningfully instrument arithmetic wrap-around conditions and catch them when they are unexpected, regardless of whether they are signed[2], unsigned[3], or pointer[4] types. Refactor open-coded wrap-around addition test to use add_would_overflow(). This paves the way to enabling the wrap-around sanitizers in the future. Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1] Link: https://github.com/KSPP/linux/issues/26 [2] Link: https://github.com/KSPP/linux/issues/27 [3] Link: https://github.com/KSPP/linux/issues/344 [4] Cc: Wei Liu Cc: Paul Durrant Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: xen-devel@lists.xenproject.org Cc: net...@vger.kernel.org Signed-off-by: Kees Cook --- drivers/net/xen-netback/hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c index ff96f22648ef..69b03b4feba9 100644 --- a/drivers/net/xen-netback/hash.c +++ b/drivers/net/xen-netback/hash.c @@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len, .flags = GNTCOPY_source_gref }}; - if ((off + len < off) || (off + len > vif->hash.size) || + if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) || len > XEN_PAGE_SIZE / sizeof(*mapping)) return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; -- 2.34.1
Re: [PATCH 80/82] xen-netback: Refactor intentional wrap-around test
On Tue, Jan 23, 2024 at 08:55:44AM +0100, Jan Beulich wrote: > On 23.01.2024 01:27, Kees Cook wrote: > > --- a/drivers/net/xen-netback/hash.c > > +++ b/drivers/net/xen-netback/hash.c > > @@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 > > gref, u32 len, > > .flags = GNTCOPY_source_gref > > }}; > > > > - if ((off + len < off) || (off + len > vif->hash.size) || > > + if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) || > > I'm not maintainer of this code, but if I was I would ask that the > excess parentheses be removed, to improve readability. Good call. I will adjust that. Thanks! -Kees -- Kees Cook
[PATCH] xen/gntalloc: Replace UAPI 1-element array
Without changing the structure size (since it is UAPI), add a proper
flexible array member, and reference it in the kernel so that it will
not be trip the array-bounds sanitizer[1].
Link: https://github.com/KSPP/linux/issues/113 [1]
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Oleksandr Tyshchenko
Cc: Gustavo A. R. Silva
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Kees Cook
---
drivers/xen/gntalloc.c | 2 +-
include/uapi/xen/gntalloc.h | 5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 26ffb8755ffb..f93f73ecefee 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -317,7 +317,7 @@ static long gntalloc_ioctl_alloc(struct
gntalloc_file_private_data *priv,
rc = -EFAULT;
goto out_free;
}
- if (copy_to_user(arg->gref_ids, gref_ids,
+ if (copy_to_user(arg->gref_ids_flex, gref_ids,
sizeof(gref_ids[0]) * op.count)) {
rc = -EFAULT;
goto out_free;
diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
index 48d2790ef928..3109282672f3 100644
--- a/include/uapi/xen/gntalloc.h
+++ b/include/uapi/xen/gntalloc.h
@@ -31,7 +31,10 @@ struct ioctl_gntalloc_alloc_gref {
__u64 index;
/* The grant references of the newly created grant, one per page */
/* Variable size, depending on count */
- __u32 gref_ids[1];
+ union {
+ __u32 gref_ids[1];
+ __DECLARE_FLEX_ARRAY(__u32, gref_ids_flex);
+ };
};
#define GNTALLOC_FLAG_WRITABLE 1
--
2.34.1
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][next] xen/xenbus: Add __counted_by for struct read_buffer and use struct_size()
On Mon, Oct 09, 2023 at 12:55:30PM -0600, Gustavo A. R. Silva wrote: > 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 via CONFIG_UBSAN_BOUNDS (for > array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > While there, use struct_size() helper, instead of the open-coded > version, to calculate the size for the allocation of the whole > flexible structure, including of course, the flexible-array member. > > This code was found with the help of Coccinelle, and audited and > fixed manually. > > Signed-off-by: Gustavo A. R. Silva Looks good. There are going to be lots of 1-byte flex array members... Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 5/9] KVM: x86: Add new hypercall to lock control registers
On Mon, May 29, 2023 at 06:48:03PM +0200, Mickaël Salaün wrote: > > On 08/05/2023 23:11, Wei Liu wrote: > > On Fri, May 05, 2023 at 05:20:42PM +0200, Mickaël Salaün wrote: > > > This enables guests to lock their CR0 and CR4 registers with a subset of > > > X86_CR0_WP, X86_CR4_SMEP, X86_CR4_SMAP, X86_CR4_UMIP, X86_CR4_FSGSBASE > > > and X86_CR4_CET flags. > > > > > > The new KVM_HC_LOCK_CR_UPDATE hypercall takes two arguments. The first > > > is to identify the control register, and the second is a bit mask to > > > pin (i.e. mark as read-only). > > > > > > These register flags should already be pinned by Linux guests, but once > > > compromised, this self-protection mechanism could be disabled, which is > > > not the case with this dedicated hypercall. > > > > > > Cc: Borislav Petkov > > > Cc: Dave Hansen > > > Cc: H. Peter Anvin > > > Cc: Ingo Molnar > > > Cc: Kees Cook > > > Cc: Madhavan T. Venkataraman > > > Cc: Paolo Bonzini > > > Cc: Sean Christopherson > > > Cc: Thomas Gleixner > > > Cc: Vitaly Kuznetsov > > > Cc: Wanpeng Li > > > Signed-off-by: Mickaël Salaün > > > Link: https://lore.kernel.org/r/20230505152046.6575-6-...@digikod.net > > [...] > > > hw_cr4 = (cr4_read_shadow() & X86_CR4_MCE) | (cr4 & > > > ~X86_CR4_MCE); > > > if (is_unrestricted_guest(vcpu)) > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index ffab64d08de3..a529455359ac 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -7927,11 +7927,77 @@ static unsigned long emulator_get_cr(struct > > > x86_emulate_ctxt *ctxt, int cr) > > > return value; > > > } > > > +#ifdef CONFIG_HEKI > > > + > > > +extern unsigned long cr4_pinned_mask; > > > + > > > > Can this be moved to a header file? > > Yep, but I'm not sure which one. Any preference Kees? Uh, er, I was never expecting that mask to be non-static. ;) To that end, how about putting it in arch/x86/kvm/x86.h ? -- Kees Cook
[PATCH] xen/mcelog: Add __nonstring annotations for unterminated strings
When a character array without a terminating NUL character has a static
initializer, GCC 15's -Wunterminated-string-initialization will only
warn if the array lacks the "nonstring" attribute[1]. Mark the arrays
with __nonstring to and correctly identify the char array as "not a C
string" and thereby eliminate the warning.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Oleksandr Tyshchenko
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Kees Cook
---
include/xen/interface/xen-mca.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/xen/interface/xen-mca.h b/include/xen/interface/xen-mca.h
index 464aa6b3a5f9..1c9afbe8cc26 100644
--- a/include/xen/interface/xen-mca.h
+++ b/include/xen/interface/xen-mca.h
@@ -372,7 +372,7 @@ struct xen_mce {
#define XEN_MCE_LOG_LEN 32
struct xen_mce_log {
- char signature[12]; /* "MACHINECHECK" */
+ char signature[12] __nonstring; /* "MACHINECHECK" */
unsigned len; /* = XEN_MCE_LOG_LEN */
unsigned next;
unsigned flags;
--
2.34.1
