On Mon, 28 Feb 2011 15:09:00 -0600
Steve French <smfrench@xxxxxxxxx> wrote:
> On Mon, Feb 28, 2011 at 12:59 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Fri, 25 Feb 2011 12:24:17 -0600
> > Steve French <smfrench@xxxxxxxxx> wrote:
> >
> >> commit 355e57ca063338eb00ea067a7570bb5f136cc513
> >> Author: Steve French <sfrench@xxxxxxxxxx>
> >> 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@xxxxxxxxxx>
> >
> > [...]
> >
> >> 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@xxxxxxxxxx>
> >
>
> 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
