Re: [PATCH 2/3] ipc: merge ipc_rcu and kern_ipc_perm

[Kees Cook]

On Mon, May 15, 2017 at 10:19 AM, Manfred Spraul
<manfred@xxxxxxxxxxxxxxxx> wrote:
> ipc has two management structures that exist for every id:
> - struct kern_ipc_perm, it contains e.g. the permissions.
> - struct ipc_rcu, it contains the rcu head for rcu handling and
>   the refcount.
>
> The patch merges both structures.
> As a bonus, we may save one cacheline, because both structures are
> cacheline aligned.
> In addition, it reduces the number of casts, instead most codepaths can
> use container_of.
>
> Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
> ---
>  include/linux/ipc.h |  3 +++
>  ipc/msg.c           | 22 ++++++++++++++--------
>  ipc/sem.c           | 35 ++++++++++++++++++++---------------
>  ipc/shm.c           | 21 ++++++++++++++-------
>  ipc/util.c          | 33 +++++++++++++++------------------
>  ipc/util.h          | 18 +++++++-----------
>  6 files changed, 73 insertions(+), 59 deletions(-)
>
> diff --git a/include/linux/ipc.h b/include/linux/ipc.h
> index 71fd92d..5591f055 100644
> --- a/include/linux/ipc.h
> +++ b/include/linux/ipc.h
> @@ -20,6 +20,9 @@ struct kern_ipc_perm {
>         umode_t         mode;
>         unsigned long   seq;
>         void            *security;
> +
> +       struct rcu_head rcu;
> +       atomic_t refcount;
>  } ____cacheline_aligned_in_smp;
>
>  #endif /* _LINUX_IPC_H */
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 104926d..e9785c4 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -97,8 +97,8 @@ static inline void msg_rmid(struct ipc_namespace *ns, struct msg_queue *s)
>
>  static void msg_rcu_free(struct rcu_head *head)
>  {
> -       struct ipc_rcu *p = container_of(head, struct ipc_rcu, rcu);
> -       struct msg_queue *msq = ipc_rcu_to_struct(p);
> +       struct kern_ipc_perm *p = container_of(head, struct kern_ipc_perm, rcu);
> +       struct msg_queue *msq = container_of(p, struct msg_queue, q_perm);
>
>         security_msg_queue_free(msq);
>         ipc_rcu_free(head);
> @@ -118,7 +118,13 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
>         key_t key = params->key;
>         int msgflg = params->flg;
>
> -       msq = ipc_rcu_alloc(sizeof(*msq));
> +       if (offsetof(struct msg_queue, q_perm) != 0) {
> +               pr_err("Invalid struct sem_perm, failing msgget().\n");
> +               return -ENOMEM;
> +       }

This is a compile-time check that will result in a runtime warning.
This should be a BUILD_BUG_ON() instead. It means we can't randomize
the first element of these structures, but I'll deal with that in the
randstruct plugin (or send another fix when I dig through it).

It looks like the "as first element" is needed to share the alloc/put
routines. To randomize the position of kern_ipc_perm within each
structure means having different accessor functions, which makes this
less fun. :P

Anyway, regardless, these patches make the sem_base thing go away and
consolidates other stuff which looks good to me.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security