diff mbox series

exportfs: fix unexporting of '/'

Message ID 20210322210238.96915-1-omosnace@redhat.com (mailing list archive)
State New
Headers show
Series exportfs: fix unexporting of '/' | expand

Commit Message

Ondrej Mosnacek March 22, 2021, 9:02 p.m. UTC
The code that has been added to strip trailing slashes from path in
unexportfs_parsed() forgot to account for the case of the root
directory, which is simply '/'. In that case it accesses path[-1] and
reduces the path to an empty string, which then fails to match any
export.

Fix it by stopping the stripping when the path is just a single
character - it doesn't matter if it's a '/' or not, we want to keep it
either way in that case.

Reproducer:

    exportfs localhost:/
    exportfs -u localhost:/

Without this patch, the unexport step fails with "exportfs: Could not
find 'localhost:/' to unexport."

Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 utils/exportfs/exportfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

J. Bruce Fields March 25, 2021, 7:19 p.m. UTC | #1
On Mon, Mar 22, 2021 at 10:02:38PM +0100, Ondrej Mosnacek wrote:
> The code that has been added to strip trailing slashes from path in
> unexportfs_parsed() forgot to account for the case of the root
> directory, which is simply '/'. In that case it accesses path[-1] and
> reduces the path to an empty string, which then fails to match any
> export.
> 
> Fix it by stopping the stripping when the path is just a single
> character - it doesn't matter if it's a '/' or not, we want to keep it
> either way in that case.

Makes sense to me.

(Note nfs-exporting / is often a bad idea.  I assume you had some good
reason in this case.)

--b.

> 
> Reproducer:
> 
>     exportfs localhost:/
>     exportfs -u localhost:/
> 
> Without this patch, the unexport step fails with "exportfs: Could not
> find 'localhost:/' to unexport."
> 
> Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  utils/exportfs/exportfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index 262dd19a..1aedd3d6 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose)
>  	 * so need to deal with it.
>  	*/
>  	size_t nlen = strlen(path);
> -	while (path[nlen - 1] == '/')
> +	while (nlen > 1 && path[nlen - 1] == '/')
>  		nlen--;
>  
>  	for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) {
> -- 
> 2.30.2
Ondrej Mosnacek March 25, 2021, 7:48 p.m. UTC | #2
On Thu, Mar 25, 2021 at 8:27 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Mar 22, 2021 at 10:02:38PM +0100, Ondrej Mosnacek wrote:
> > The code that has been added to strip trailing slashes from path in
> > unexportfs_parsed() forgot to account for the case of the root
> > directory, which is simply '/'. In that case it accesses path[-1] and
> > reduces the path to an empty string, which then fails to match any
> > export.
> >
> > Fix it by stopping the stripping when the path is just a single
> > character - it doesn't matter if it's a '/' or not, we want to keep it
> > either way in that case.
>
> Makes sense to me.
>
> (Note nfs-exporting / is often a bad idea.  I assume you had some good
> reason in this case.)

I hit it in a test, so if the only issue is that it exposes more than
necessary, then I guess it's fine in this case :)

>
> --b.
>
> >
> > Reproducer:
> >
> >     exportfs localhost:/
> >     exportfs -u localhost:/
> >
> > Without this patch, the unexport step fails with "exportfs: Could not
> > find 'localhost:/' to unexport."
> >
> > Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()")
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  utils/exportfs/exportfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> > index 262dd19a..1aedd3d6 100644
> > --- a/utils/exportfs/exportfs.c
> > +++ b/utils/exportfs/exportfs.c
> > @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose)
> >        * so need to deal with it.
> >       */
> >       size_t nlen = strlen(path);
> > -     while (path[nlen - 1] == '/')
> > +     while (nlen > 1 && path[nlen - 1] == '/')
> >               nlen--;
> >
> >       for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) {
> > --
> > 2.30.2
>


--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
J. Bruce Fields March 25, 2021, 8:09 p.m. UTC | #3
On Thu, Mar 25, 2021 at 08:48:57PM +0100, Ondrej Mosnacek wrote:
> On Thu, Mar 25, 2021 at 8:27 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Mon, Mar 22, 2021 at 10:02:38PM +0100, Ondrej Mosnacek wrote:
> > > The code that has been added to strip trailing slashes from path in
> > > unexportfs_parsed() forgot to account for the case of the root
> > > directory, which is simply '/'. In that case it accesses path[-1] and
> > > reduces the path to an empty string, which then fails to match any
> > > export.
> > >
> > > Fix it by stopping the stripping when the path is just a single
> > > character - it doesn't matter if it's a '/' or not, we want to keep it
> > > either way in that case.
> >
> > Makes sense to me.
> >
> > (Note nfs-exporting / is often a bad idea.  I assume you had some good
> > reason in this case.)
> 
> I hit it in a test, so if the only issue is that it exposes more than
> necessary, then I guess it's fine in this case :)

Yes, in a lot of typical setups, exporting "/" would expose files to the
network that you don't really want to.

But, anyway, it should work.  Looks like a good fix.

--b.

> 
> >
> > --b.
> >
> > >
> > > Reproducer:
> > >
> > >     exportfs localhost:/
> > >     exportfs -u localhost:/
> > >
> > > Without this patch, the unexport step fails with "exportfs: Could not
> > > find 'localhost:/' to unexport."
> > >
> > > Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()")
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  utils/exportfs/exportfs.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> > > index 262dd19a..1aedd3d6 100644
> > > --- a/utils/exportfs/exportfs.c
> > > +++ b/utils/exportfs/exportfs.c
> > > @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose)
> > >        * so need to deal with it.
> > >       */
> > >       size_t nlen = strlen(path);
> > > -     while (path[nlen - 1] == '/')
> > > +     while (nlen > 1 && path[nlen - 1] == '/')
> > >               nlen--;
> > >
> > >       for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) {
> > > --
> > > 2.30.2
> >
> 
> 
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
Steve Dickson April 7, 2021, 6:28 p.m. UTC | #4
On 3/22/21 5:02 PM, Ondrej Mosnacek wrote:
> The code that has been added to strip trailing slashes from path in
> unexportfs_parsed() forgot to account for the case of the root
> directory, which is simply '/'. In that case it accesses path[-1] and
> reduces the path to an empty string, which then fails to match any
> export.
> 
> Fix it by stopping the stripping when the path is just a single
> character - it doesn't matter if it's a '/' or not, we want to keep it
> either way in that case.
> 
> Reproducer:
> 
>     exportfs localhost:/
>     exportfs -u localhost:/
> 
> Without this patch, the unexport step fails with "exportfs: Could not
> find 'localhost:/' to unexport."
> 
> Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Committed... (tag: nfs-utils-2-5-4-rc2)

steved.

> ---
>  utils/exportfs/exportfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index 262dd19a..1aedd3d6 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose)
>  	 * so need to deal with it.
>  	*/
>  	size_t nlen = strlen(path);
> -	while (path[nlen - 1] == '/')
> +	while (nlen > 1 && path[nlen - 1] == '/')
>  		nlen--;
>  
>  	for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) {
>
diff mbox series

Patch

diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index 262dd19a..1aedd3d6 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -383,7 +383,7 @@  unexportfs_parsed(char *hname, char *path, int verbose)
 	 * so need to deal with it.
 	*/
 	size_t nlen = strlen(path);
-	while (path[nlen - 1] == '/')
+	while (nlen > 1 && path[nlen - 1] == '/')
 		nlen--;
 
 	for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) {