Re: [GIT] [CIFS] Allow user names longer than 32 bytes

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Mon, 28 Feb 2011 15:09:00 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Feb 28, 2011 at 12:59 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 25 Feb 2011 12:24:17 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> commit 355e57ca063338eb00ea067a7570bb5f136cc513
> >> Author: Steve French <sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >> Date:   Fri Feb 25 01:11:56 2011 -0600
> >>
> >>     [CIFS] Allow user names longer than 32 bytes
> >>
> >>     We artificially limited the user name to 32 bytes, but modern servers handle
> >>     larger.  Set the maximum length to a reasonable 256, and make the user name
> >>     string dynamically allocated rather than a fixed size in session structure.
> >>     Also clean up old checkpatch warning.
> >>
> >>     Signed-off-by: Steve French <sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >
> > [...]
> >
> >> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> >> index a51585f..e307a28 100644
> >> --- a/fs/cifs/cifsencrypt.c
> >> +++ b/fs/cifs/cifsencrypt.c
> >> @@ -469,15 +469,15 @@ static int calc_ntlmv2_hash(struct cifsSesInfo
> >> *ses, char *ntlmv2_hash,
> >>               return rc;
> >>       }
> >>
> >> -     /* convert ses->userName to unicode and uppercase */
> >> -     len = strlen(ses->userName);
> >> +     /* convert ses->user_name to unicode and uppercase */
> >> +     len = strlen(ses->user_name);
> >>       user = kmalloc(2 + (len * 2), GFP_KERNEL);
> >>       if (user == NULL) {
> >>               cERROR(1, "calc_ntlmv2_hash: user mem alloc failure\n");
> >>               rc = -ENOMEM;
> >>               goto calc_exit_2;
> >>       }
> >> -     len = cifs_strtoUCS((__le16 *)user, ses->userName, len, nls_cp);
> >> +     len = cifs_strtoUCS((__le16 *)user, ses->user_name, len, nls_cp);
> >>       UniStrupr(user);
> >>
> >
> > Now that user_name can be a NULL pointer, what prevents the code above
> > from oopsing if no one passes in a user= parm?
> >
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> 
> In cifs_mount in fs/cifs/connect.c we guarantee that
> volume_info->username is filled in (or we exit mount)
> 
> 	if (volume_info->nullauth) {
> 		cFYI(1, "null user");
> 		volume_info->username = "";
> 	} else if (volume_info->username) {
> 		/* BB fixme parse for domain name here */
> 		cFYI(1, "Username: %s", volume_info->username);
> 	} else {
> 		cifserror("No username specified");
> 	/* In userspace mount helper we can get user name from alternate
> 	   locations such as env variables and files on disk */
> 		rc = -EINVAL;
> 		goto out;
> 	}
> 
> then in get_smb_ses (see below) when we allocate a new session we
> either kstrdup successfully or we fail the mount:
> 	if (volume_info->username) {
> 		ses->user_name = kstrdup(volume_info->username, GFP_KERNEL);
> 		if (!ses->user_name)
> 			goto get_ses_fail;
> 	}
> 
> 
> I don't mind adding defensive code in other places, but the reason the
> null check is (only) in the find_cifs_smb_ses is in case an invalid
> ses (being freed) was on the list.

That would be a bug and I think there would be better ways to check for
that if you think it's a possibility. I think we need to clearly define
the rules for this, and make the code follow those rules. There are two
possibilities:

1) it's possible to have a NULL user_name pointer. You always need to
check for that and deal with it appropriately.

...or...

2) it's not ever possible to have a NULL user_name pointer. Once the
ses has been created, NULL pointer checks are unneeded.

I'm ok with either, but I think it needs to be consistent. Checking for
a NULL pointer in only some places is very confusing.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>