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]
Message-ID: <CACT4Y+YbFBfB9xM2bjbxick42ZFQPTa_oQ4ejcmL=ASsZ2GY6A@mail.gmail.com>
Date:   Mon, 9 Jul 2018 20:31:35 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Manfred Spraul <manfred@...orfullife.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.

On Mon, Jul 9, 2018 at 8:22 PM, Manfred Spraul <manfred@...orfullife.com> wrote:
> 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

Right!

> 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
>