On Thu, May 25, 2017 at 12:34 PM, Kees Cook <keesc...@chromium.org> wrote:
> On Thu, May 25, 2017 at 11:50 AM, Manfred Spraul
> <manf...@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
