Re: [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.

[Manfred Spraul]

Hello Dmitry,

On 07/09/2018 07:05 PM, Dmitry Vyukov wrote:
> On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manfred@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> > ---
> >   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