[linux-cifs-client] [PATCH] [CIFS] Fixed race: Read freed memory when accessing cifs_sb->prepath

[Igor Mammedov]

Jeff Layton wrote:
> On Mon, 06 Oct 2008 19:51:08 +0400
> Igor Mammedov <niallain at gmail.com> wrote:
> 
>> >From a902e94f1e603855797a56237a30726e088f3f05 Mon Sep 17 00:00:00 2001  
>> From: Igor Mammedov <niallain at gmail.com>
>> Date: Mon, 6 Oct 2008 18:28:21 +0400
>> Subject: [PATCH] [CIFS] Fixed race: Read freed memory when accessing cifs_sb->prepath
>>
>> Signed-off-by: Igor Mammedov <niallain at gmail.com>
>> ---
>>  fs/cifs/connect.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 4c13bcd..0665b89 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -2308,13 +2308,15 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>>  	the password ptr is put in the new session structure (in which case the
>>  	password will be freed at unmount time) */
>>  out:
>> +	if (rc)
>> +		kfree(volume_info.prepath);
>> +
>>  	/* zero out password before freeing */
>>  	if (volume_info.password != NULL) {
>>  		memset(volume_info.password, 0, strlen(volume_info.password));
>>  		kfree(volume_info.password);
>>  	}
>>  	kfree(volume_info.UNC);
>> -	kfree(volume_info.prepath);
>>  	FreeXid(xid);
>>  	return rc;
>>  }
> 
> I don't see any race here. Can you explain how this happens? The only place
> we reference volume_info.prepath is here:
> 
>                 /* calculate prepath */
>                 cifs_sb->prepath = volume_info.prepath;
>                 if (cifs_sb->prepath) {
>                         cifs_sb->prepathlen = strlen(cifs_sb->prepath);
>                         /* we can not convert the / to \ in the path
>                         separators in the prefixpath yet because we do not
>                         know (until reset_cifs_unix_caps is called later)
>                         whether POSIX PATH CAP is available. We normalize
>                         the / to \ after reset_cifs_unix_caps is called */
>                         volume_info.prepath = NULL;
> 
> The pointer should be NULL after this, and that should make the kfree
> harmless.

Sorry I've not noticed:
	'volume_info.prepath = NULL;'



-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com