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.
--
Thanks,
Steve
--
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
