| Message ID | 20230517225838.never.965-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | ad0ff58dc959b138f8125ce6679fbd1ec8784f83 |
| Headers | show |
| Series | pid: Replace struct pid 1-element array with flex-array | expand |
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 <brauner@kernel.org> > Cc: Jan Kara <jack@suse.cz> > Cc: Andreas Gruenbacher <agruenba@redhat.com> > Cc: Daniel Verkamp <dverkamp@chromium.org> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Jeff Xu <jeffxu@google.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: Frederic Weisbecker <frederic@kernel.org> > Reported-by: syzbot+ac3b41786a2d0565b6d5@syzkaller.appspotmail.com > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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 <brauner@kernel.org> 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 <jeffxu@google.com> > 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
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)
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 <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Andreas Gruenbacher <agruenba@redhat.com> Cc: Daniel Verkamp <dverkamp@chromium.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Cc: Jeff Xu <jeffxu@google.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Boqun Feng <boqun.feng@gmail.com> Cc: Luis Chamberlain <mcgrof@kernel.org> Cc: Frederic Weisbecker <frederic@kernel.org> Reported-by: syzbot+ac3b41786a2d0565b6d5@syzkaller.appspotmail.com Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/pid.h | 2 +- kernel/pid.c | 13 ++++++++----- kernel/pid_namespace.c | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-)