From: Stuart Henderson <stu@spacehopper.org>
Subject: Re: [patch] mount_mfs allow setting permissions manually
To: Rafael Sadowski <rafael@sizeofvoid.org>, tech@openbsd.org
Date: Mon, 21 Jul 2025 18:48:06 +0100

On 2025/07/21 04:18, Crystal Kolipe wrote:
> On Sun, Jul 20, 2025 at 12:59:09PM +0100, Stuart Henderson wrote:
> > On 2025/07/20 04:52, Crystal Kolipe wrote:
> > > Do we also need to clarify in the man page what happens if both -p and -P are
> > > used at the same time?  In that case, -p is ignored and the permissions from
> > > the directory specified with -P are used.
> > > 
> > > I'm wondering if that is even the desired behaviour, maybe -p should always
> > > override even when -P is specified.  But if nobody has an opinion on this
> > > I'm OK with the current proposal.
> > 
> > fwiw, I think I would expect -p to be used "over the top" of -P's
> > permissions if both flags are specified.
> 
> This version should address that.  Now if both -p and -P are specified on the
> command line, the permissions of '.' are set to whatever was specified as the
> argument to -p.
> 
> I'm assuming that's what you meant, although "over the top" could also be
> interpreted as 'a logical AND of both the permissions gained from -P and those
> specified with -p', (which could in itself could be considered an over the top
> suggestion in another sense, thereby causing an infinite mental loop :-) ).
> 
> The chmod of '.' is not expected to fail, so we only print a warning if it
> does, and continue with the inherited permissions.  Should this be a hard
> error?

That seems reasonable. fwiw, the code bits are OK with me.

I'm not sure about the text about "can create security issues" in
the manual though, I think I'd stick to just describing the action
of -p.




> --- newfs.c.orig	Mon Apr  7 04:25:02 2025
> +++ newfs.c	Mon Jul 21 08:15:41 2025
> @@ -147,7 +147,7 @@
>  static void waitformount(char *, pid_t);
>  static int do_exec(const char *, const char *, char *const[]);
>  static int isdir(const char *);
> -static void copy(char *, char *);
> +static void copy(char *, char *, mode_t);
>  static int gettmpmnt(char *, size_t);
>  #endif
>  
> @@ -179,6 +179,7 @@
>  #ifdef MFS
>  	char mountfromname[BUFSIZ];
>  	char *pop = NULL, node[PATH_MAX];
> +	char *ep;
>  	pid_t pid;
>  	struct stat mountpoint;
>  #endif
> @@ -203,7 +204,7 @@
>  		fatal("insane maxpartitions value %d", maxpartitions);
>  
>  	opstring = mfs ?
> -	    "O:P:T:b:c:e:f:i:m:o:s:" :
> +	    "O:P:T:b:c:e:f:i:m:o:p:s:" :
>  	    "NO:S:T:b:c:e:f:g:h:i:m:o:qs:t:";
>  	while ((ch = getopt(argc, argv, opstring)) != -1) {
>  		switch (ch) {
> @@ -286,6 +287,19 @@
>  					    optarg);
>  			}
>  			break;
> +		case 'p':
> +			errno = 0;
> +			mfsmode = strtoul(optarg, &ep, 8);
> +			if (optarg[0] == '\0' || *ep != '\0')
> +				fatal("invalid mode: not a number");
> +			if (errno == ERANGE)
> +				fatal("invalid mode: out of range");
> +			if ((mfsmode & ALLPERMS) != mfsmode)
> +				fatal("unrecognised permission bits");
> +			if (mfsmode == 0)
> +				warnx("invalid mode, will inherit "
> +				      "mount point permissions");
> +			break;
>  		case 'q':
>  			quiet = 1;
>  			break;
> @@ -506,7 +520,8 @@
>  			err(ECANCELED, "stat %s", node);
>  		mfsuid = mountpoint.st_uid;
>  		mfsgid = mountpoint.st_gid;
> -		mfsmode = mountpoint.st_mode & ALLPERMS;
> +		if (mfsmode == 0)
> +			mfsmode = mountpoint.st_mode & ALLPERMS;
>  	}
>  #endif
>  
> @@ -543,7 +558,7 @@
>  		default:
>  			if (pop != NULL) {
>  				waitformount(tmpnode, pid);
> -				copy(pop, tmpnode);
> +				copy(pop, tmpnode, mfsmode);
>  				unmount(tmpnode, 0);
>  				rmdir(tmpnode);
>  			}
> @@ -754,13 +769,18 @@
>  }
>  
>  static void
> -copy(char *src, char *dst)
> +copy(char *src, char *dst, mode_t mfsmode)
>  {
>  	int ret, dir, created = 0;
> +	int fd;
>  	struct ufs_args mount_args;
>  	char mountpoint[MNAMELEN];
>  	char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
>  
> +	if ((fd = open(dst, O_RDONLY)) == -1) {
> +		err(1, "failed opening %s", dst);
> +	}
> +
>  	dir = isdir(src);
>  	if (dir)
>  		strlcpy(mountpoint, src, sizeof(mountpoint));
> @@ -788,6 +808,10 @@
>  			warn("unmount %s", dst);
>  		errx(1, "copy %s to %s failed", mountpoint, dst);
>  	}
> +	if (fchmodat(fd, ".", mfsmode, 0) == -1) {
> +		warn("failed to set requested permissions");
> +	}
> +	close (fd);
>  }
>  
>  static int
>