| Message ID | 20221118114137.128088-1-arefev@swemel.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] namespace: Added pointer check in copy_mnt_ns() | expand |
On Fri, Nov 18, 2022 at 02:41:37PM +0300, Denis Arefev wrote: > Return value of a function 'next_mnt' is dereferenced at > namespace.c:3377 without checking for null, > but it is usually checked for this function > > Found by Linux Verification Center (linuxtesting.org) with SVACE. NAK. I see a bug in there, but it's not going to trigger a NULL pointer dereference and your patch doesn't fix it at all. That loop ought to be // skipped mntns binding? while (p->mnt.mnt_root != q->mnt.mnt_root) p = next_mnt(skip_mnt_tree(p), old); and I suspect that it'll confuse your tool even worse. What happens here is that new tree is congruent to the old one, with some subtrees skipped. Each node N in the new tree is a clone of some node (Origin(N)) in the old one. Copying preserves node order. We want to have p == Origin(q) on each iteration. What we really have (due to the real bug) is p is no later than Origin(q) in node ordering Initially it's trivially true (p points to root of the old tree, and the only way it would *not* be copied would be to somehow get mntns binding as root; in that case copy_tree() would've failed and we wouldn't get to that loop at all). Suppose it is true on some iteration. What happens on the next one? q hadn't been the last node in the new tree, or we would've found next_mnt(q, new) to be NULL and exited the loop. But that means that p "<=" Origin(q) "<" Origin(next_mnt(q, new)) ("<" and "<=" in the node ordering, that is). So p couldn't have been the last node in the old tree and next_mnt(p, old) "<=" Origin(next_mnt(q, new)) After the p = next_mnt(p, old); q = next_mnt(q, new); if (!q) break; we have p != NULL && p "<=" Origin(q) Cloning preserves ->mnt_root, so the subsequent loop while (p->mnt.mnt_root != q->mnt.mnt_root) p = next_mnt(p, old); could be rewritten as while (p->mnt.mnt_root != Origin(q)->mnt.mnt_root) p = next_mnt(p, old); and in that form it's really obvious that p will not advance past Origin(q), nevermind running out of nodes. So on the next iteration the property still holds. There's no way for your added checks to trigger. There *IS* a bug in that logics, though - mntns binding can have a file bound on top of it. In such case it is possible to have p behind the Origin(q) for a (short) while. It's not going to cause serious problems, but that's certainly a non-obvious behaviour and a comment needed to explain why it's not problem is certainly longer than the one-liner change eliminating the oddity. Note that running into mnt_root mismatch means that p is currently pointing to mntns binding we'd skipped when copying. So let's skip the subtree in the same way copy_tree() did... The bottom line: * your NULL pointer checks could never trigger; if you *do* have a reproducer, please post it. * there's a (pretty harmless) bug in that code, but it is not fixed by your patch. * see if your tool is any happier with the patch below; I would be rather surprised if it did, but... diff --git a/fs/namespace.c b/fs/namespace.c index df137ba19d37..c80f422084eb 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3515,8 +3515,9 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns, q = next_mnt(q, new); if (!q) break; + // an mntns binding we'd skipped? while (p->mnt.mnt_root != q->mnt.mnt_root) - p = next_mnt(p, old); + p = next_mnt(skip_mnt_tree(p), old); } namespace_unlock();
diff --git a/fs/namespace.c b/fs/namespace.c index cebaa3e81794..06472a110257 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3348,9 +3348,9 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns, } p = next_mnt(p, old); q = next_mnt(q, new); - if (!q) + if (!q || !p) break; - while (p->mnt.mnt_root != q->mnt.mnt_root) + while (p && (p->mnt.mnt_root != q->mnt.mnt_root)) p = next_mnt(p, old); } namespace_unlock();
Return value of a function 'next_mnt' is dereferenced at namespace.c:3377 without checking for null, but it is usually checked for this function Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Denis Arefev <arefev@swemel.ru> --- fs/namespace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)