Re: [PATCH 02/20] ipc: merge ipc_rcu and kern_ipc_perm

[Kees Cook <keescook@chromium.org>]

On Thu, May 25, 2017 at 12:34 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, May 25, 2017 at 11:50 AM, Manfred Spraul
> <manfred@colorfullife.com> 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.
>>
>> To simplify code, the ipc_rcu_alloc initializes the allocation to 0.
>
> I don't see this change in the code, only the removal of sem's
> memset(..., 0, ...)?
>
>> diff --git a/ipc/sem.c b/ipc/sem.c
>> index fff8337..bdff6d9 100644
>> --- a/ipc/sem.c
>> +++ b/ipc/sem.c
>> @@ -469,20 +469,20 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
>>         if (ns->used_sems + nsems > ns->sc_semmns)
>>                 return -ENOSPC;
>>
>> +       BUILD_BUG_ON(offsetof(struct sem_array, sem_perm) != 0);
>> +
>>         size = sizeof(*sma) + nsems * sizeof(sma->sems[0]);
>> -       sma = ipc_rcu_alloc(size);
>> +       sma = container_of(ipc_rcu_alloc(size), struct sem_array, sem_perm);
>>         if (!sma)
>>                 return -ENOMEM;
>>
>> -       memset(sma, 0, size);
>> -
>>         sma->sem_perm.mode = (semflg & S_IRWXUGO);
>>         sma->sem_perm.key = key;
>>
>>         sma->sem_perm.security = NULL;
>>         retval = security_sem_alloc(sma);
>>         if (retval) {
>> -               ipc_rcu_putref(sma, ipc_rcu_free);
>> +               ipc_rcu_putref(&sma->sem_perm, ipc_rcu_free);
>>                 return retval;
>>         }
>>
>> [...]
>> diff --git a/ipc/util.c b/ipc/util.c
>> index caec7b1..9dcc08b 100644
>> --- a/ipc/util.c
>> +++ b/ipc/util.c
>> @@ -418,46 +418,43 @@ void ipc_free(void *ptr)
>>  }
>>
>>  /**
>> - * ipc_rcu_alloc - allocate ipc and rcu space
>> + * ipc_rcu_alloc - allocate ipc space
>>   * @size: size desired
>>   *
>> - * Allocate memory for the rcu header structure +  the object.
>> - * Returns the pointer to the object or NULL upon failure.
>> + * Allocate memory for an ipc object.
>> + * The first member must be struct kern_ipc_perm.
>>   */
>> -void *ipc_rcu_alloc(int size)
>> +struct kern_ipc_perm *ipc_rcu_alloc(int size)
>>  {
>>         /*
>>          * We prepend the allocation with the rcu struct
>>          */
>> -       struct ipc_rcu *out = ipc_alloc(sizeof(struct ipc_rcu) + size);
>> +       struct kern_ipc_perm *out = ipc_alloc(size);
>>         if (unlikely(!out))
>>                 return NULL;
>>         atomic_set(&out->refcount, 1);
>> -       return out + 1;
>> +       return out;
>>  }

See above:

- newary() loses memset()
- ipc_rcu_alloc() does not gain it
- changelog says "To simplify code, the ipc_rcu_alloc initializes the
allocation to 0." This is what's wanted but the patch doesn't do it.

The actual change that (temporarily) adds memset to ipc_rcu_alloc() is
one of the following patches, but it should be here or bisection may
cause unexpected results for semaphores.

It's a minor issue, since these will likely all land at the same time,
but it's probably still worth fixing.

-Kees

-- 
Kees Cook
Pixel Security