bin/54829: rename operation crashes mount_9p if target file already exists

[idrarig.alocin%gmail.com@localhost]

>Number:         54829
>Category:       bin
>Synopsis:       rename operation crashes mount_9p if target file already exists
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 04 11:15:00 +0000 2020
>Originator:     Nicola Girardi
>Release:        8.1
>Organization:
>Environment:
NetBSD hope 8.1 NetBSD 8.1 (GENERIC) #0: Fri May 31 08:43:59 UTC 2019  mkrepro%mkrepro.NetBSD.org@localhost:/usr/src/sys/arch/amd64/compile/GENERIC amd64
>Description:
I was trying to use a 9P fs in NetBSD and found mount_9p
crashes when on the rename op when the target file already exists.

AFAICS the first problem is passing pn_targ->pn_data instead of pn_targ to noderemove(), causing invalid memory access in noderemove().

The second problem seems to be the call to puffs_setback() from the rename handler, which fails an assertion and aborts the file server. Since mount_psshfs does not call puffs_setback() in the analogous situation (it also has to remove before rename if the target already exists), I've removed the call here as well, in my proposed fix below. I haven't studied the internals enough to be confident about the patch and can only say that it works for me.
>How-To-Repeat:
While I found the problem by cloning a git repository on a mounted 9P fs, this would be the minimal reproducer:

mount_9p yourhost /yourmnt
cd /yourmnt
touch a
touch b
mv a b

>Fix:
Proposed fix but needs more scrutiny.

; cvs diff -u
cvs diff: Diffing .
Index: node.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/puffs/mount_9p/node.c,v
retrieving revision 1.21
diff -u -r1.21 node.c
--- node.c      18 Jan 2009 10:10:47 -0000      1.21
+++ node.c      4 Jan 2020 10:05:31 -0000
@@ -516,11 +516,7 @@
                p9n->fid_base = P9P_INVALFID;
                puffs_pn_remove(pn);
        }
-
- out:
-       if (rv == 0)
-               puffs_setback(pcc, PUFFS_SETBACK_NOREF_N2);
-
+out:
        RETURN(rv);
 }

@@ -528,24 +524,34 @@
 puffs9p_node_remove(struct puffs_usermount *pu, void *opc, void *targ,
        const struct puffs_cn *pcn)
 {
+       struct puffs_cc *pcc = puffs_cc_getcc(pu);
+       int rv = 0;
        struct puffs_node *pn = targ;

        if (pn->pn_va.va_type == VDIR)
                return EISDIR;

-       return noderemove(pu, pn);
+       rv = noderemove(pu, pn);
+       if (rv == 0)
+               puffs_setback(pcc, PUFFS_SETBACK_NOREF_N2);
+       return rv;
 }

 int
 puffs9p_node_rmdir(struct puffs_usermount *pu, void *opc, void *targ,
        const struct puffs_cn *pcn)
 {
+       struct puffs_cc *pcc = puffs_cc_getcc(pu);
+       int rv = 0;
        struct puffs_node *pn = targ;

        if (pn->pn_va.va_type != VDIR)
                return ENOTDIR;

-       return noderemove(pu, pn);
+       rv = noderemove(pu, pn);
+       if (rv == 0)
+               puffs_setback(pcc, PUFFS_SETBACK_NOREF_N2);
+       return rv;
 }

 /*
@@ -571,7 +577,7 @@
        if (targ) {
                struct puffs_node *pn_targ = targ;

-               rv = noderemove(pu, pn_targ->pn_data);
+               rv = noderemove(pu, pn_targ);
                if (rv)
                        goto out;
        }