linux-kernel - Re: [PATCH 12/12] ipc/util.c: Further ipc_idr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Jul 2018 20:22:27 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        LKML <linux-kernel@...r.kernel.org>, 1vier1@....de,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.

Hello Dmitry,

On 07/09/2018 07:05 PM, Dmitry Vyukov wrote:
> On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manfred@...orfullife.com> wrote:
>> If idr_alloc within ipc_idr_alloc fails, then the return value (-ENOSPC)
>> is used to calculate new->id.
>> Technically, this is not a bug, because new->id is never accessed.
>>
>> But: Clean it up anyways: On error, just return, do not set new->id.
>> And improve the documentation.
>>
>> Signed-off-by: Manfred Spraul <manfred@...orfullife.com>
>> Cc: Dmitry Vyukov <dvyukov@...gle.com>
>> ---
>>   ipc/util.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/ipc/util.c b/ipc/util.c
>> index d474f2b3b299..302c18fc846b 100644
>> --- a/ipc/util.c
>> +++ b/ipc/util.c
>> @@ -182,11 +182,20 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
>>   }
>>
>>   /*
>> - * Specify desired id for next allocated IPC object.
>> + * Insert new IPC object into idr tree, and set sequence number and id
>> + * in the correct order.
>> + * Especially:
>> + * - the sequence number must be set before inserting the object into the idr,
>> + *   because the sequence number is accessed without a lock.
>> + * - the id can/must be set after inserting the object into the idr.
>> + *   All accesses must be done after getting kern_ipc_perm.lock.
>> + *
>> + * The caller must own kern_ipc_perm.lock.of the new object.
>> + * On error, the function returns a (negative) error code.
>>    */
>>   static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>>   {
>> -       int key, next_id = -1;
>> +       int id, next_id = -1;
> /\/\/\/\
> Looks good to me. I was also confused by how key transforms into id,
> and then key name is used for something else.
Let's see if there are further findings, perhaps I'll rework the series, 
it may make sense to standardize the variable names:

id: user space id. Called semid, shmid, msgid if the type is known.
     Most functions use "id" already.
     Exception: ipc_checkid(), the function calls is uid.
idx: "index" for the idr lookup
     Right now, ipc_rmid() use lid, ipc_addid() use id as variable name
seq: sequence counter, to avoid quick collisions of the user space id
     In the comments, it got a mixture of sequence counter and sequence 
number.
key: user space key, used for the rhash tree

>>   #ifdef CONFIG_CHECKPOINT_RESTORE
>>          next_id = ids->next_id;
>> @@ -197,14 +206,15 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>>                  new->seq = ids->seq++;
>>                  if (ids->seq > IPCID_SEQ_MAX)
>>                          ids->seq = 0;
>> -               key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>> +               id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>>          } else {
>>                  new->seq = ipcid_to_seqx(next_id);
>> -               key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
>> +               id = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
>>                                  0, GFP_NOWAIT);
>>          }
>> -       new->id = SEQ_MULTIPLIER * new->seq + key;
>> -       return key;
>> +       if (id >= 0)
>> +               new->id = SEQ_MULTIPLIER * new->seq + id;
> We still initialize seq in this case. I guess it's ok because the
> object is not published at all. But if we are doing this, then perhaps
> store seq into a local var first and then:
>
>        if (id >= 0) {
>                new->id = SEQ_MULTIPLIER * seq + id;
>                new->seq = seq:
>        }
>
> ?
No!!!
We must initialize ->seq before publication. Otherwise we end up with 
the syzcall findings, or in the worst case a strange rare failure of an 
ipc operation.
The difference between ->id and ->seq is that we have the valid number 
for ->seq.

For the user space ID we cannot have the valid number unless the 
idr_alloc is successful.
The patch only avoids that this line is executed:

> 	new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)

As I wrote, the line shouldn't cause any damage, the code is more or less:
> 	new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
> 	kfree(new);
But this is ugly, it asks for problems.

--
	Manfred