Re: unveil pathname pool

[Alexander Bluhm]

On Tue, Aug 06, 2019 at 06:57:49AM +0200, Sebastien Marie wrote:
> On Mon, Aug 05, 2019 at 07:21:22PM +0200, Alexander Bluhm wrote:
> > unveil(2) allocates 1024 bytes on the stack.  That is a lot.  Better
> > use namei pool like sys___realpath() does.
>
> There is a missing pool_put() in early return.
>
>        999 #ifdef KTRACE
>       1000         if (KTRPOINT(p, KTR_STRUCT))
>       1001                 ktrstruct(p, "unveil", permissions, 
> strlen(permissions));
>       1002 #endif
>       1003         if (pathlen < 2)
>       1004                 return EINVAL;
>
> else it seems fine.
>
> ok semarie@ with that added.

oops, how could I miss that?

Perhaps a goto end is nicer then.  sys___realpath does that also.

ok?

bluhm

Index: kern/vfs_syscalls.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.332
diff -u -p -r1.332 vfs_syscalls.c
--- kern/vfs_syscalls.c 5 Aug 2019 23:28:55 -0000       1.332
+++ kern/vfs_syscalls.c 6 Aug 2019 19:47:29 -0000
@@ -975,7 +975,7 @@ sys_unveil(struct proc *p, void *v, regi
                syscallarg(const char *) path;
                syscallarg(const char *) permissions;
        } */ *uap = v;
-       char pathname[MAXPATHLEN], *c;
+       char *pathname, *c;
        struct nameidata nd;
        size_t pathlen;
        char permissions[5];
@@ -992,17 +992,20 @@ sys_unveil(struct proc *p, void *v, regi
        error = copyinstr(SCARG(uap, permissions), permissions,
            sizeof(permissions), NULL);
        if (error)
-               return(error);
-       error = copyinstr(SCARG(uap, path), pathname, sizeof(pathname), 
&pathlen);
+               return (error);
+       pathname = pool_get(&namei_pool, PR_WAITOK);
+       error = copyinstr(SCARG(uap, path), pathname, MAXPATHLEN, &pathlen);
        if (error)
-               return(error);
+               goto end;

 #ifdef KTRACE
        if (KTRPOINT(p, KTR_STRUCT))
                ktrstruct(p, "unveil", permissions, strlen(permissions));
 #endif
-       if (pathlen < 2)
-               return EINVAL;
+       if (pathlen < 2) {
+               error = EINVAL;
+               goto end;
+       }

        /* find root "/" or "//" */
        for (c = pathname; *c != '\0'; c++) {
@@ -1019,7 +1022,7 @@ sys_unveil(struct proc *p, void *v, regi

        nd.ni_pledge = PLEDGE_UNVEIL;
        if ((error = namei(&nd)) != 0)
-               goto end;
+               goto ndfree;

        /*
         * XXX Any access to the file or directory will allow us to
@@ -1059,8 +1062,10 @@ sys_unveil(struct proc *p, void *v, regi
                vrele(nd.ni_dvp);

        pool_put(&namei_pool, nd.ni_cnd.cn_pnbuf);
-end:
+ndfree:
        unveil_free_traversed_vnodes(&nd);
+end:
+       pool_put(&namei_pool, pathname);

        return (error);
 }