On Thu, Jul 5, 2018 at 7:59 AM, Manfred Spraul <manf...@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+2827ef6b3385deb07...@syzkaller.appspotmail.com
> Signed-off-by: Manfred Spraul <manf...@colorfullife.com>
> Cc: Dmitry Vyukov <dvyu...@google.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Davidlohr Bueso <d...@stgolabs.net>
> Cc: Michael Kerrisk <mtk.manpa...@gmail.com>
> Signed-off-by: Manfred Spraul <manf...@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 <dvyu...@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;
}
