On Wed, Feb 22, 2023 at 10:19:57AM -0700, Todd C. Miller wrote:
> On Wed, 22 Feb 2023 17:11:47 +0100, Moritz Buhl wrote:
>
> > The tool finds a path to ex_search where fsb.f_fsid is uninitialized.
> >
> > ex_search compares the potentially uninitialized stack data:
> >
> > ex_search(fsid_t *fsid)
> > {
> > struct exportlist *ep;
> >
> > ep = exphead;
> > while (ep) {
> > if (ep->ex_fs.val[0] == fsid->val[0] &&
> > ...
> >
> > Is it sufficient to zero fsb?
> > Is this really reachable?
>
> I believe that it is. However, I don't know why we wouldn't just
> want to skip ex_search, etc when bad is set. Also, there is no
> reason to use a critical section now that the SIGHUP handle just
> sets a flag. Something like this (untested), should be fine.
The code checker still finds an issue.
if (realpath(rpcpath, dirpath) == NULL) {
bad = errno;
...
}
if (bad)
goto mount_error;
This relys on the fact that after realpath() == NULL, errno != 0.
I have checked libc, this works. The new report is a false positive.
mbuhl@ mentioned to me that
strlcpy(dirpath, rpcpath, sizeof(dirpath));
is dead code now. Before dirpath was used in
((dp = dirp_search(ep->ex_dirl, dirpath)) &&
even in the bad case.
Now bad is set to ENOENT in a case where EACCES was used before.
I doubt that this little change has relevant impact.
OK bluhm@
> It needs testing by someone who actually uses NFS though...
I am running it on my NFS server.
> Index: sbin/mountd/mountd.c
> ===================================================================
> RCS file: /cvs/src/sbin/mountd/mountd.c,v
> retrieving revision 1.89
> diff -u -p -u -r1.89 mountd.c
> --- sbin/mountd/mountd.c 6 Aug 2020 17:57:32 -0000 1.89
> +++ sbin/mountd/mountd.c 22 Feb 2023 17:17:44 -0000
> @@ -736,7 +736,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
> char rpcpath[RPCMNT_PATHLEN+1], dirpath[PATH_MAX];
> struct hostent *hp = NULL;
> struct exportlist *ep;
> - sigset_t sighup_mask;
> int defset, hostset;
> struct fhreturn fhr;
> struct dirlist *dp;
> @@ -746,8 +745,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
> u_short sport;
> long bad = 0;
>
> - sigemptyset(&sighup_mask);
> - sigaddset(&sighup_mask, SIGHUP);
> saddr = transp->xp_raddr.sin_addr.s_addr;
> sport = ntohs(transp->xp_raddr.sin_port);
> switch (rqstp->rq_proc) {
> @@ -790,9 +787,10 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
> fprintf(stderr, "stat failed on %s\n", dirpath);
> bad = ENOENT; /* We will send error reply later */
> }
> + if (bad)
> + goto mount_error;
>
> /* Check in the exports list */
> - sigprocmask(SIG_BLOCK, &sighup_mask, NULL);
> ep = ex_search(&fsb.f_fsid);
> hostset = defset = 0;
> if (ep && (chk_host(ep->ex_defdir, saddr, &defset, &hostset) ||
> @@ -800,13 +798,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
> chk_host(dp, saddr, &defset, &hostset)) ||
> (defset && scan_tree(ep->ex_defdir, saddr) == 0 &&
> scan_tree(ep->ex_dirl, saddr) == 0))) {
> - if (bad) {
> - if (!svc_sendreply(transp, xdr_long,
> - (caddr_t)&bad))
> - syslog(LOG_ERR, "Can't send reply");
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
> - return;
> - }
> if (hostset & DP_HOSTSET)
> fhr.fhr_flag = hostset;
> else
> @@ -817,11 +808,7 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
> if (imsg_getfh(dirpath, (fhandle_t *)&fhr.fhr_fh) < 0) {
> bad = errno;
> syslog(LOG_ERR, "Can't get fh for %s", dirpath);
> - if (!svc_sendreply(transp, xdr_long,
> - (caddr_t)&bad))
> - syslog(LOG_ERR, "Can't send reply");
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
> - return;
> + goto mount_error;
> }
> if (!svc_sendreply(transp, xdr_fhs, (caddr_t)&fhr))
> syslog(LOG_ERR, "Can't send reply");
> @@ -842,9 +829,11 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
> } else
> bad = EACCES;
>
> - if (bad && !svc_sendreply(transp, xdr_long, (caddr_t)&bad))
> - syslog(LOG_ERR, "Can't send reply");
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
> + if (bad) {
> + mount_error:
> + if (!svc_sendreply(transp, xdr_long, (caddr_t)&bad))
> + syslog(LOG_ERR, "Can't send reply");
> + }
> return;
> case RPCMNT_DUMP:
> if (!svc_sendreply(transp, xdr_mlist, NULL))
> @@ -958,11 +947,7 @@ xdr_explist(XDR *xdrsp, caddr_t cp)
> {
> struct exportlist *ep;
> int false = 0, putdef;
> - sigset_t sighup_mask;
>
> - sigemptyset(&sighup_mask);
> - sigaddset(&sighup_mask, SIGHUP);
> - sigprocmask(SIG_BLOCK, &sighup_mask, NULL);
> ep = exphead;
> while (ep) {
> putdef = 0;
> @@ -973,12 +958,9 @@ xdr_explist(XDR *xdrsp, caddr_t cp)
> goto errout;
> ep = ep->ex_next;
> }
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
> - if (!xdr_bool(xdrsp, &false))
> - return (0);
> - return (1);
> + if (xdr_bool(xdrsp, &false))
> + return (1);
> errout:
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
> return (0);
> }
>
> @@ -1050,7 +1032,6 @@ void
> new_exportlist(int signo)
> {
> gothup = 1;
> -
> }
>
> /*
