The patch titled
Subject: ipc: reorganize initialization of kern_ipc_perm.seq
has been added to the -mm tree. Its filename is
ipc-reorganize-initialization-of-kern_ipc_permseq.patch
This patch should soon appear at
http://ozlabs.org/~akpm/mmots/broken-out/ipc-reorganize-initialization-of-kern_ipc_permseq.patch
and later at
http://ozlabs.org/~akpm/mmotm/broken-out/ipc-reorganize-initialization-of-kern_ipc_permseq.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next and is updated
there every 3-4 working days
------------------------------------------------------
From: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
Subject: ipc: reorganize initialization of kern_ipc_perm.seq
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, it
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.
Link: http://lkml.kernel.org/r/20180709151019.1336-3-manfred@xxxxxxxxxxxxxxxx
Reported-by: syzbot+2827ef6b3385deb07eaf@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Cc: Kees Cook <keescook@xxxxxxxxxxxx>
Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
Cc: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
Documentation/sysctl/kernel.txt | 3 +-
ipc/util.c | 45 +++++++++++++++++++++---------
2 files changed, 34 insertions(+), 14 deletions(-)
diff -puN Documentation/sysctl/kernel.txt~ipc-reorganize-initialization-of-kern_ipc_permseq Documentation/sysctl/kernel.txt
--- a/Documentation/sysctl/kernel.txt~ipc-reorganize-initialization-of-kern_ipc_permseq
+++ a/Documentation/sysctl/kernel.txt
@@ -453,7 +453,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 -puN ipc/util.c~ipc-reorganize-initialization-of-kern_ipc_permseq ipc/util.c
--- a/ipc/util.c~ipc-reorganize-initialization-of-kern_ipc_permseq
+++ a/ipc/util.c
@@ -197,13 +197,24 @@ static struct kern_ipc_perm *ipc_findkey
/*
* 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, st
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, struc
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, struc
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;
}
_
Patches currently in -mm which might be from manfred@xxxxxxxxxxxxxxxx are
ipc-reorganize-initialization-of-kern_ipc_permid.patch
ipc-reorganize-initialization-of-kern_ipc_permseq.patch
ipc-utilc-use-ipc_rcu_putref-for-failues-in-ipc_addid.patch
ipc-rename-ipcctl_pre_down_nolock.patch
ipc-utilc-correct-comment-in-ipc_obtain_object_check.patch
ipc-rename-ipc_lock-to-ipc_lock_idr.patch
ipc-utilc-further-ipc_idr_alloc-cleanups.patch
--
To unsubscribe from this list: send the line "unsubscribe mm-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
