Messages in this thread Patch in this message | | | From | Dmitry Vyukov <> | Date | Thu, 5 Jul 2018 10:36:20 +0200 | Subject | Re: [PATCH 2/6] ipc: reorganize initialization of kern_ipc_perm.seq |
| |
On Thu, Jul 5, 2018 at 7:59 AM, Manfred Spraul <manfred@colorfullife.com> wrote: > ipc_addid() initializes kern_ipc_perm.seq after having called > ipc_idr_alloc(). > > Thus a parallel semop() or msgrcv() that uses ipc_obtain_object_check() > may see an uninitialized value. > > The patch moves the initialization of kern_ipc_perm.seq before the > calls of ipc_idr_alloc(). > > Notes: > 1) This patch has a user space visible side effect: > If /proc/sys/kernel/*_next_id is used (i.e.: checkpoint/restore) and > if semget()/msgget()/shmget() fails in the final step of adding the id > to the rhash tree, then .._next_id is cleared. Before the patch, is > remained unmodified. > > There is no change of the behavior after a successful ..get() call: > It always clears .._next_id, there is no impact to non checkpoint/restore > code as that code does not use .._next_id. > > 2) The patch correctly documents that after a call to ipc_idr_alloc(), > the full tear-down sequence must be used. The callers of ipc_addid() > do not fullfill that, i.e. more bugfixes are required. > > Reported-by: syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com > Signed-off-by: Manfred Spraul <manfred@colorfullife.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Michael Kerrisk <mtk.manpages@gmail.com> > Signed-off-by: Manfred Spraul <manfred@colorfullife.com> > --- > Documentation/sysctl/kernel.txt | 3 ++- > ipc/util.c | 45 +++++++++++++++++++++++---------- > 2 files changed, 34 insertions(+), 14 deletions(-) > > diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt > index eded671d55eb..b2d4a8f8fe97 100644 > --- a/Documentation/sysctl/kernel.txt > +++ b/Documentation/sysctl/kernel.txt > @@ -440,7 +440,8 @@ Notes: > 1) kernel doesn't guarantee, that new object will have desired id. So, > it's up to userspace, how to handle an object with "wrong" id. > 2) Toggle with non-default value will be set back to -1 by kernel after > -successful IPC object allocation. > +successful IPC object allocation. If an IPC object allocation syscall > +fails, it is undefined if the value remains unmodified or is reset to -1. > > ============================================================== > > diff --git a/ipc/util.c b/ipc/util.c > index 4e81182fa0ac..662c28c6c9fa 100644 > --- a/ipc/util.c > +++ b/ipc/util.c > @@ -197,13 +197,24 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key) > /* > * Specify desired id for next allocated IPC object. > */ > -#define ipc_idr_alloc(ids, new) \ > - idr_alloc(&(ids)->ipcs_idr, (new), \ > - (ids)->next_id < 0 ? 0 : ipcid_to_idx((ids)->next_id),\ > - 0, GFP_NOWAIT) > +static inline int ipc_idr_alloc(struct ipc_ids *ids, > + struct kern_ipc_perm *new) > +{ > + int key; > > -static inline int ipc_buildid(int id, struct ipc_ids *ids, > - struct kern_ipc_perm *new) > + if (ids->next_id < 0) { > + key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT); > + } else { > + key = idr_alloc(&ids->ipcs_idr, new, > + ipcid_to_idx(ids->next_id), > + 0, GFP_NOWAIT); > + ids->next_id = -1; > + } > + return key; > +} > + > +static inline void ipc_set_seq(struct ipc_ids *ids, > + struct kern_ipc_perm *new) > { > if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */ > new->seq = ids->seq++; > @@ -211,24 +222,19 @@ static inline int ipc_buildid(int id, struct ipc_ids *ids, > ids->seq = 0; > } else { > new->seq = ipcid_to_seqx(ids->next_id); > - ids->next_id = -1; > } > - > - return SEQ_MULTIPLIER * new->seq + id; > } > > #else > #define ipc_idr_alloc(ids, new) \ > idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT) > > -static inline int ipc_buildid(int id, struct ipc_ids *ids, > +static inline void ipc_set_seq(struct ipc_ids *ids, > struct kern_ipc_perm *new) > { > new->seq = ids->seq++; > if (ids->seq > IPCID_SEQ_MAX) > ids->seq = 0; > - > - return SEQ_MULTIPLIER * new->seq + id; > } > > #endif /* CONFIG_CHECKPOINT_RESTORE */ > @@ -270,6 +276,19 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) > new->cuid = new->uid = euid; > new->gid = new->cgid = egid; > > + ipc_set_seq(ids, new); > + > + /* > + * As soon as a new object is inserted into the idr, > + * ipc_obtain_object_idr() or ipc_obtain_object_check() can find it, > + * and the lockless preparations for ipc operations can start. > + * This means especially: permission checks, audit calls, allocation > + * of undo structures, ... > + * > + * Thus the object must be fully initialized, and if something fails, > + * then the full tear-down sequence must be followed. > + * (i.e.: set new->deleted, reduce refcount, call_rcu()) > + */ > id = ipc_idr_alloc(ids, new); > idr_preload_end(); > > @@ -291,7 +310,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) > if (id > ids->max_id) > ids->max_id = id; > > - new->id = ipc_buildid(id, ids, new); > + new->id = SEQ_MULTIPLIER * new->seq + id; > > return id; > }
Hi Manfred,
The series looks like a significant improvement to me. Thanks!
I feel that this code can be further simplified (unless I am missing something here). Please take a look at this version:
https://github.com/dvyukov/linux/commit/f77aeaf80f3c4ab524db92184d874b03063fea3a?diff=split
This is on top of your patches. It basically does the same as your code, but consolidates all id/seq assignment and dealing with next_id, and deduplicates code re CONFIG_CHECKPOINT_RESTORE. Currently it's a bit tricky to follow e.g. where exactly next_id is consumed and where it needs to be left intact. The only difference is that my code assigns new->id earlier. Not sure if it can lead to anything bad. But if yes, then it seems that currently uninitialized new->id is exposed. If necessary (?) we could reset new->id in the same place where we set new->deleted.
Other patches in the series look good to me.
p.s. I've uploaded full context diffs of all patches here: https://github.com/dvyukov/linux/commits/ipc1 commit f77aeaf80f3c4ab524db92184d874b03063fea3a Author: Dmitry Vyukov <dvyukov@google.com> Date: Thu Jul 5 10:15:26 2018 +0200
ipc_idr_alloc refactoring
diff --git a/ipc/util.c b/ipc/util.c index 776a9ce2905f..c98675d8612e 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -193,52 +193,32 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key) return NULL; } -#ifdef CONFIG_CHECKPOINT_RESTORE /* * Specify desired id for next allocated IPC object. */ -static inline int ipc_idr_alloc(struct ipc_ids *ids, - struct kern_ipc_perm *new) +static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) { - int key; + int key, next_id = -1; - if (ids->next_id < 0) { - key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT); - } else { - key = idr_alloc(&ids->ipcs_idr, new, - ipcid_to_idx(ids->next_id), - 0, GFP_NOWAIT); - ids->next_id = -1; - } - return key; -} +#ifdef CONFIG_CHECKPOINT_RESTORE + next_id = ids->next_id; + ids->next_id = -1; +#endif -static inline void ipc_set_seq(struct ipc_ids *ids, - struct kern_ipc_perm *new) -{ - if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */ + if (next_id < 0) { /* !CHECKPOINT_RESTORE or next_id is unset */ new->seq = ids->seq++; if (ids->seq > IPCID_SEQ_MAX) ids->seq = 0; + key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT); } else { - new->seq = ipcid_to_seqx(ids->next_id); + new->seq = ipcid_to_seqx(next_id); + key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id), + 0, GFP_NOWAIT); } + new->id = SEQ_MULTIPLIER * new->seq + key; + return key; } -#else -#define ipc_idr_alloc(ids, new) \ - idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT) - -static inline void ipc_set_seq(struct ipc_ids *ids, - struct kern_ipc_perm *new) -{ - new->seq = ids->seq++; - if (ids->seq > IPCID_SEQ_MAX) - ids->seq = 0; -} - -#endif /* CONFIG_CHECKPOINT_RESTORE */ - /** * ipc_addid - add an ipc identifier * @ids: ipc identifier set @@ -278,8 +258,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) current_euid_egid(&euid, &egid); new->cuid = new->uid = euid; new->gid = new->cgid = egid; - - ipc_set_seq(ids, new); new->deleted = false; /* @@ -317,9 +295,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) ids->in_use++; if (id > ids->max_id) ids->max_id = id; - - new->id = SEQ_MULTIPLIER * new->seq + id; - return id; } | |