Message-ID: <20081215223005.00d3ee23@tupile.poochiereds.net>
Date:	Mon, 15 Dec 2008 22:30:05 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	"Steve French" <smfrench@...il.com>
Cc:	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-cifs-client@...ts.samba.org, niallain@...il.com,
	smfrench@...tin.rr.com
Subject: Re: [linux-cifs-client] Re: [PATCH] cifs: make sure that DFS
 pathnames are properly formed

On Mon, 15 Dec 2008 21:18:29 -0600
"Steve French" <smfrench@...il.com> wrote:

> This fix looks correct and urgent, although if we have time I would
> like to try it for another day before requesting merge to linux-2.6
> 

I think it's correct. I'm not sure if I'd term it urgent. It only means
that DFS referrals (which are still labeled experimental) aren't working,
and only in the case of samba servers (AFAICT). Windows DFS referrals
still seem to work OK without this path.

Still, it's probably safe enough if you do want to push it for 2.6.28.

> The dfs documentation does state that we should be sending \ rather
> than \\ (although earlier places in the document stated the reverse,
> it is more clear later in the network documentation).
> On Mon, Dec 15, 2008 at 7:02 PM, Jeff Layton <jlayton@...hat.com> wrote:
> > The paths in a DFS request are supposed to only have a single preceding
> > backslash, but we are sending them with a double backslash. This is
> > exposing a bug in Windows where it also sends a path in the response
> > that has a double backslash.
> >
> > The existing code that builds the mount option string however expects a
> > double backslash prefix in a couple of places when it tries to use the
> > path returned by build_path_from_dentry. Fix compose_mount_options to
> > expect properly formed DFS paths (single backslash at front).
> >
> > Also clean up error handling in that function. There was a possible
> > NULL pointer dereference and situations where a partially built option
> > string would be returned.
> >
> > Tested against Samba 3.0.28-ish server and Win2k8.
> >
> > Signed-off-by: Jeff Layton <jlayton@...hat.com>
> > ---
> >  fs/cifs/cifs_dfs_ref.c |   48 ++++++++++++++++++++++++++++++++++++------------
> >  1 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> > index e1c1836..85c0a74 100644
> > --- a/fs/cifs/cifs_dfs_ref.c
> > +++ b/fs/cifs/cifs_dfs_ref.c
> > @@ -122,7 +122,7 @@ static char *compose_mount_options(const char *sb_mountdata,
> >                                   char **devname)
> >  {
> >        int rc;
> > -       char *mountdata;
> > +       char *mountdata = NULL;
> >        int md_len;
> >        char *tkn_e;
> >        char *srvIP = NULL;
> > @@ -136,10 +136,9 @@ static char *compose_mount_options(const char *sb_mountdata,
> >        *devname = cifs_get_share_name(ref->node_name);
> >        rc = dns_resolve_server_name_to_ip(*devname, &srvIP);
> >        if (rc != 0) {
> > -               cERROR(1, ("%s: Failed to resolve server part of %s to IP",
> > -                         __func__, *devname));
> > -               mountdata = ERR_PTR(rc);
> > -               goto compose_mount_options_out;
> > +               cERROR(1, ("%s: Failed to resolve server part of %s to IP: %d",
> > +                         __func__, *devname, rc));;
> > +               goto compose_mount_options_err;
> >        }
> >        /* md_len = strlen(...) + 12 for 'sep+prefixpath='
> >         * assuming that we have 'unc=' and 'ip=' in
> > @@ -149,8 +148,8 @@ static char *compose_mount_options(const char *sb_mountdata,
> >                strlen(ref->node_name) + 12;
> >        mountdata = kzalloc(md_len+1, GFP_KERNEL);
> >        if (mountdata == NULL) {
> > -               mountdata = ERR_PTR(-ENOMEM);
> > -               goto compose_mount_options_out;
> > +               rc = -ENOMEM;
> > +               goto compose_mount_options_err;
> >        }
> >
> >        /* copy all options except of unc,ip,prefixpath */
> > @@ -197,18 +196,32 @@ static char *compose_mount_options(const char *sb_mountdata,
> >
> >        /* find & copy prefixpath */
> >        tkn_e = strchr(ref->node_name + 2, '\\');
> > -       if (tkn_e == NULL) /* invalid unc, missing share name*/
> > -               goto compose_mount_options_out;
> > +       if (tkn_e == NULL) {
> > +               /* invalid unc, missing share name*/
> > +               rc = -EINVAL;
> > +               goto compose_mount_options_err;
> > +       }
> >
> > +       /*
> > +        * this function gives us a path with a double backslash prefix. We
> > +        * require a single backslash for DFS. Temporarily increment fullpath
> > +        * to put it in the proper form and decrement before freeing it.
> > +        */
> >        fullpath = build_path_from_dentry(dentry);
> > +       if (!fullpath) {
> > +               rc = -ENOMEM;
> > +               goto compose_mount_options_err;
> > +       }
> > +       ++fullpath;
> >        tkn_e = strchr(tkn_e + 1, '\\');
> > -       if (tkn_e || strlen(fullpath) - (ref->path_consumed)) {
> > +       if (tkn_e || (strlen(fullpath) - ref->path_consumed)) {
> >                strncat(mountdata, &sep, 1);
> >                strcat(mountdata, "prefixpath=");
> >                if (tkn_e)
> >                        strcat(mountdata, tkn_e + 1);
> > -               strcat(mountdata, fullpath + (ref->path_consumed));
> > +               strcat(mountdata, fullpath + ref->path_consumed);
> >        }
> > +       --fullpath;
> >        kfree(fullpath);
> >
> >        /*cFYI(1,("%s: parent mountdata: %s", __func__,sb_mountdata));*/
> > @@ -217,6 +230,11 @@ static char *compose_mount_options(const char *sb_mountdata,
> >  compose_mount_options_out:
> >        kfree(srvIP);
> >        return mountdata;
> > +
> > +compose_mount_options_err:
> > +       kfree(mountdata);
> > +       mountdata = ERR_PTR(rc);
> > +       goto compose_mount_options_out;
> >  }
> >
> >
> > @@ -309,13 +327,19 @@ cifs_dfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
> >                goto out_err;
> >        }
> >
> > +       /*
> > +        * The MSDFS spec states that paths in DFS referral requests and
> > +        * responses must be prefixed by a single '\' character instead of
> > +        * the double backslashes usually used in the UNC. This function
> > +        * gives us the latter, so we must adjust the result.
> > +        */
> >        full_path = build_path_from_dentry(dentry);
> >        if (full_path == NULL) {
> >                rc = -ENOMEM;
> >                goto out_err;
> >        }
> >
> > -       rc = get_dfs_path(xid, ses , full_path, cifs_sb->local_nls,
> > +       rc = get_dfs_path(xid, ses , full_path + 1, cifs_sb->local_nls,
> >                &num_referrals, &referrals,
> >                cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> >
> > --
> > 1.5.5.1
> >
> >
> 
> 
> 
> -- 
> Thanks,
> 
> Steve
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...ts.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 


-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists