2019-06-03 17:53:39

by Trond Myklebust

Subject: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

When attempting to strip the root path, we should first canonicalise
the root pathname.

Signed-off-by: Trond Myklebust <[email protected]>
---
support/misc/nfsd_path.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
index 2f41a793c534..9b38dd96007f 100644
--- a/support/misc/nfsd_path.c
+++ b/support/misc/nfsd_path.c
@@ -1,6 +1,7 @@
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
+#include <limits.h>
#include <stdlib.h>
#include <unistd.h>

@@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
char *
nfsd_path_strip_root(char *pathname)
{
+ char buffer[PATH_MAX];
const char *dir = nfsd_path_nfsd_rootdir();
char *ret;

- ret = strstr(pathname, dir);
- if (!ret || ret != pathname)
- return pathname;
- return pathname + strlen(dir);
+ if (!dir)
+ goto out;
+ if (realpath(dir, buffer)) {
+ ret = strstr(pathname, buffer);
+ if (ret == pathname)
+ return pathname + strlen(dir);
+ } else
+ xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
+ __func__, dir);
+out:
+ return pathname;
}

char *
--
2.21.0

2019-06-03 17:54:04

by Trond Myklebust

Subject: [PATCH 3/3] mountd: Canonicalise the rootdir in exportent_mkrealpath()

Ensure that we canonicalise the export path when generating the
real path.

Signed-off-by: Trond Myklebust <[email protected]>
---
support/export/export.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/support/export/export.c b/support/export/export.c
index 82bbb54c5e9e..c753f68e4d63 100644
--- a/support/export/export.c
+++ b/support/export/export.c
@@ -14,6 +14,7 @@
#include <sys/types.h>
#include <sys/param.h>
#include <netinet/in.h>
+#include <limits.h>
#include <stdlib.h>
#include <dirent.h>
#include <errno.h>
@@ -21,6 +22,7 @@
#include "nfslib.h"
#include "exportfs.h"
#include "nfsd_path.h"
+#include "xlog.h"

exp_hash_table exportlist[MCL_MAXTYPES] = {{NULL, {{NULL,NULL}, }}, };
static int export_hash(char *);
@@ -38,8 +40,14 @@ exportent_mkrealpath(struct exportent *eep)
const char *chroot = nfsd_path_nfsd_rootdir();
char *ret = NULL;

- if (chroot)
- ret = nfsd_path_prepend_dir(chroot, eep->e_path);
+ if (chroot) {
+ char buffer[PATH_MAX];
+ if (realpath(chroot, buffer))
+ ret = nfsd_path_prepend_dir(buffer, eep->e_path);
+ else
+ xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
+ __func__, chroot);
+ }
if (!ret)
ret = xstrdup(eep->e_path);
eep->e_realpath = ret;
--
2.21.0

2019-06-04 15:46:40

by J. Bruce Fields

Subject: Re: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

On Mon, Jun 03, 2019 at 01:12:26PM -0400, Trond Myklebust wrote:
> When attempting to strip the root path, we should first canonicalise
> the root pathname.
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> support/misc/nfsd_path.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
> index 2f41a793c534..9b38dd96007f 100644
> --- a/support/misc/nfsd_path.c
> +++ b/support/misc/nfsd_path.c
> @@ -1,6 +1,7 @@
> #include <errno.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> +#include <limits.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> @@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
> char *
> nfsd_path_strip_root(char *pathname)
> {
> + char buffer[PATH_MAX];
> const char *dir = nfsd_path_nfsd_rootdir();
> char *ret;
>
> - ret = strstr(pathname, dir);
> - if (!ret || ret != pathname)
> - return pathname;
> - return pathname + strlen(dir);
> + if (!dir)
> + goto out;
> + if (realpath(dir, buffer)) {
> + ret = strstr(pathname, buffer);
> + if (ret == pathname)
> + return pathname + strlen(dir);
> + } else
> + xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
> + __func__, dir);
> +out:
> + return pathname;

I still don't get this.

So in the case strstr doesn't find anything, it returns the path
unchanged.

That means that if the next_mnt() caller asks whether there are any
mounts underneath /rootdir/a/b, and nextdir finds a mountpoint at
/a/b/c, it can return that, right?

--b.



> }
>
> char *
> --
> 2.21.0

2019-06-04 17:59:25

by Trond Myklebust

Subject: Re: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

On Tue, 2019-06-04 at 11:46 -0400, J. Bruce Fields wrote:
> On Mon, Jun 03, 2019 at 01:12:26PM -0400, Trond Myklebust wrote:
> > When attempting to strip the root path, we should first
> > canonicalise
> > the root pathname.
> >
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > support/misc/nfsd_path.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
> > index 2f41a793c534..9b38dd96007f 100644
> > --- a/support/misc/nfsd_path.c
> > +++ b/support/misc/nfsd_path.c
> > @@ -1,6 +1,7 @@
> > #include <errno.h>
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > +#include <limits.h>
> > #include <stdlib.h>
> > #include <unistd.h>
> >
> > @@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
> > char *
> > nfsd_path_strip_root(char *pathname)
> > {
> > + char buffer[PATH_MAX];
> > const char *dir = nfsd_path_nfsd_rootdir();
> > char *ret;
> >
> > - ret = strstr(pathname, dir);
> > - if (!ret || ret != pathname)
> > - return pathname;
> > - return pathname + strlen(dir);
> > + if (!dir)
> > + goto out;
> > + if (realpath(dir, buffer)) {
> > + ret = strstr(pathname, buffer);
> > + if (ret == pathname)
> > + return pathname + strlen(dir);
> > + } else
> > + xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
> > + __func__, dir);
> > +out:
> > + return pathname;
>
> I still don't get this.
>
> So in the case strstr doesn't find anything, it returns the path
> unchanged.
>
> That means that if the next_mnt() caller asks whether there are any
> mounts underneath /rootdir/a/b, and nextdir finds a mountpoint at
> /a/b/c, it can return that, right?
>

Ack. Sending out a v2 of these patches.

Thanks Bruce!

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2019-06-04 18:01:49

by J. Bruce Fields

Subject: Re: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

On Tue, Jun 04, 2019 at 05:58:59PM +0000, Trond Myklebust wrote:
> On Tue, 2019-06-04 at 11:46 -0400, J. Bruce Fields wrote:
> > On Mon, Jun 03, 2019 at 01:12:26PM -0400, Trond Myklebust wrote:
> > > When attempting to strip the root path, we should first
> > > canonicalise
> > > the root pathname.
> > >
> > > Signed-off-by: Trond Myklebust <[email protected]>
> > > ---
> > > support/misc/nfsd_path.c | 17 +++++++++++++----
> > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
> > > index 2f41a793c534..9b38dd96007f 100644
> > > --- a/support/misc/nfsd_path.c
> > > +++ b/support/misc/nfsd_path.c
> > > @@ -1,6 +1,7 @@
> > > #include <errno.h>
> > > #include <sys/types.h>
> > > #include <sys/stat.h>
> > > +#include <limits.h>
> > > #include <stdlib.h>
> > > #include <unistd.h>
> > >
> > > @@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
> > > char *
> > > nfsd_path_strip_root(char *pathname)
> > > {
> > > + char buffer[PATH_MAX];
> > > const char *dir = nfsd_path_nfsd_rootdir();
> > > char *ret;
> > >
> > > - ret = strstr(pathname, dir);
> > > - if (!ret || ret != pathname)
> > > - return pathname;
> > > - return pathname + strlen(dir);
> > > + if (!dir)
> > > + goto out;
> > > + if (realpath(dir, buffer)) {
> > > + ret = strstr(pathname, buffer);
> > > + if (ret == pathname)
> > > + return pathname + strlen(dir);
> > > + } else
> > > + xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
> > > + __func__, dir);
> > > +out:
> > > + return pathname;
> >
> > I still don't get this.
> >
> > So in the case strstr doesn't find anything, it returns the path
> > unchanged.
> >
> > That means that if the next_mnt() caller asks whether there are any
> > mounts underneath /rootdir/a/b, and nextdir finds a mountpoint at
> > /a/b/c, it can return that, right?
> >
>
> Ack. Sending out a v2 of these patches.

Oh, good, thanks, I thought I was going crazy.

(Always a possibility, especially when I'm looking at code.)

--b.