For pid namespaces, struct pid uses a dynamically sized array member,
"numbers". This was implemented using the ancient 1-element fake flexible
array, which has been deprecated for decades. Replace it with a C99
flexible array, refactor the array size calculations to use struct_size(),
and address elements via indexes. Note that the static initializer (which
defines a single element) works as-is, and requires no special handling.
Without this, CONFIG_UBSAN_BOUNDS (and potentially CONFIG_FORTIFY_SOURCE)
will trigger bounds checks:
https://lore.kernel.org/lkml/20230517-bushaltestelle-super-e223978c1ba6@brauner
Cc: Christian Brauner <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Andreas Gruenbacher <[email protected]>
Cc: Daniel Verkamp <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Jeff Xu <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Reported-by: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/pid.h | 2 +-
kernel/pid.c | 13 ++++++++-----
kernel/pid_namespace.c | 2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index b75de288a8c2..653a527574c4 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -67,7 +67,7 @@ struct pid
/* wait queue for pidfd notifications */
wait_queue_head_t wait_pidfd;
struct rcu_head rcu;
- struct upid numbers[1];
+ struct upid numbers[];
};
extern struct pid init_struct_pid;
diff --git a/kernel/pid.c b/kernel/pid.c
index f93954a0384d..f5008146e2e4 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -131,7 +131,7 @@ void free_pid(struct pid *pid)
spin_lock_irqsave(&pidmap_lock, flags);
for (i = 0; i <= pid->level; i++) {
- struct upid *upid = pid->numbers + i;
+ struct upid *upid = &pid->numbers[i];
struct pid_namespace *ns = upid->ns;
switch (--ns->pid_allocated) {
case 2:
@@ -265,7 +265,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
init_waitqueue_head(&pid->wait_pidfd);
INIT_HLIST_HEAD(&pid->inodes);
- upid = pid->numbers + ns->level;
+ upid = &pid->numbers[ns->level];
spin_lock_irq(&pidmap_lock);
if (!(ns->pid_allocated & PIDNS_ADDING))
goto out_unlock;
@@ -285,7 +285,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
out_free:
spin_lock_irq(&pidmap_lock);
while (++i <= ns->level) {
- upid = pid->numbers + i;
+ upid = &pid->numbers[i];
idr_remove(&upid->ns->idr, upid->nr);
}
@@ -656,8 +656,11 @@ void __init pid_idr_init(void)
idr_init(&init_pid_ns.idr);
- init_pid_ns.pid_cachep = KMEM_CACHE(pid,
- SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
+ init_pid_ns.pid_cachep = kmem_cache_create("pid",
+ struct_size((struct pid *)0, numbers, 1),
+ __alignof__(struct pid),
+ SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT,
+ NULL);
}
static struct file *__pidfd_fget(struct task_struct *task, int fd)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index b43eee07b00c..ebb20091a9da 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -48,7 +48,7 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
return kc;
snprintf(name, sizeof(name), "pid_%u", level + 1);
- len = sizeof(struct pid) + level * sizeof(struct upid);
+ len = struct_size((struct pid *)0, numbers, level + 1);
mutex_lock(&pid_caches_mutex);
/* Name collision forces to do allocation under mutex. */
if (!*pkc)
--
2.34.1
On Wed, May 17, 2023 at 03:58:42PM -0700, Kees Cook wrote:
> For pid namespaces, struct pid uses a dynamically sized array member,
> "numbers". This was implemented using the ancient 1-element fake flexible
> array, which has been deprecated for decades. Replace it with a C99
> flexible array, refactor the array size calculations to use struct_size(),
> and address elements via indexes. Note that the static initializer (which
> defines a single element) works as-is, and requires no special handling.
>
> Without this, CONFIG_UBSAN_BOUNDS (and potentially CONFIG_FORTIFY_SOURCE)
> will trigger bounds checks:
> https://lore.kernel.org/lkml/20230517-bushaltestelle-super-e223978c1ba6@brauner
>
> Cc: Christian Brauner <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Andreas Gruenbacher <[email protected]>
> Cc: Daniel Verkamp <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Jeff Xu <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Reported-by: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> include/linux/pid.h | 2 +-
> kernel/pid.c | 13 ++++++++-----
> kernel/pid_namespace.c | 2 +-
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index b75de288a8c2..653a527574c4 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -67,7 +67,7 @@ struct pid
> /* wait queue for pidfd notifications */
> wait_queue_head_t wait_pidfd;
> struct rcu_head rcu;
> - struct upid numbers[1];
> + struct upid numbers[];
> };
>
> extern struct pid init_struct_pid;
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f93954a0384d..f5008146e2e4 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -131,7 +131,7 @@ void free_pid(struct pid *pid)
>
> spin_lock_irqsave(&pidmap_lock, flags);
> for (i = 0; i <= pid->level; i++) {
> - struct upid *upid = pid->numbers + i;
> + struct upid *upid = &pid->numbers[i];
> struct pid_namespace *ns = upid->ns;
> switch (--ns->pid_allocated) {
> case 2:
> @@ -265,7 +265,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
> init_waitqueue_head(&pid->wait_pidfd);
> INIT_HLIST_HEAD(&pid->inodes);
>
> - upid = pid->numbers + ns->level;
> + upid = &pid->numbers[ns->level];
> spin_lock_irq(&pidmap_lock);
> if (!(ns->pid_allocated & PIDNS_ADDING))
> goto out_unlock;
> @@ -285,7 +285,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
> out_free:
> spin_lock_irq(&pidmap_lock);
> while (++i <= ns->level) {
> - upid = pid->numbers + i;
> + upid = &pid->numbers[i];
> idr_remove(&upid->ns->idr, upid->nr);
> }
>
> @@ -656,8 +656,11 @@ void __init pid_idr_init(void)
>
> idr_init(&init_pid_ns.idr);
>
> - init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> - SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> + init_pid_ns.pid_cachep = kmem_cache_create("pid",
> + struct_size((struct pid *)0, numbers, 1),
> + __alignof__(struct pid),
> + SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT,
> + NULL);
> }
>
> static struct file *__pidfd_fget(struct task_struct *task, int fd)
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index b43eee07b00c..ebb20091a9da 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -48,7 +48,7 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
> return kc;
>
> snprintf(name, sizeof(name), "pid_%u", level + 1);
> - len = sizeof(struct pid) + level * sizeof(struct upid);
> + len = struct_size((struct pid *)0, numbers, level + 1);
Having this open-coded is a bit ugly tbh, especially the visible cast
to 0. But fine we can probably live with this since it's really only
localized to this file.
On Wed, 17 May 2023 15:58:42 -0700, Kees Cook wrote:
> For pid namespaces, struct pid uses a dynamically sized array member,
> "numbers". This was implemented using the ancient 1-element fake flexible
> array, which has been deprecated for decades. Replace it with a C99
> flexible array, refactor the array size calculations to use struct_size(),
> and address elements via indexes. Note that the static initializer (which
> defines a single element) works as-is, and requires no special handling.
>
> [...]
Applied to the kernel.fork branch of the brauner/linux.git tree.
Patches in the kernel.fork branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git
branch: kernel.fork
[1/1] pid: Replace struct pid 1-element array with flex-array
https://git.kernel.org/brauner/linux/c/ad0ff58dc959
On Thu, May 18, 2023 at 8:07 AM Christian Brauner <[email protected]> wrote:
>
> On Wed, 17 May 2023 15:58:42 -0700, Kees Cook wrote:
> > For pid namespaces, struct pid uses a dynamically sized array member,
> > "numbers". This was implemented using the ancient 1-element fake flexible
> > array, which has been deprecated for decades. Replace it with a C99
> > flexible array, refactor the array size calculations to use struct_size(),
> > and address elements via indexes. Note that the static initializer (which
> > defines a single element) works as-is, and requires no special handling.
> >
> > [...]
>
> Applied to the kernel.fork branch of the brauner/linux.git tree.
> Patches in the kernel.fork branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
Acked-by: Jeff Xu <[email protected]>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git
> branch: kernel.fork
>
> [1/1] pid: Replace struct pid 1-element array with flex-array
> https://git.kernel.org/brauner/linux/c/ad0ff58dc959