Q: cifs, freeing volume_info->UNCip

PATCH J. R. Okajima 3 participants 4 messages Aug 15, 2011 – Aug 18, 2011

Subsystems (2)

SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

J. R. Okajima

author

2011-08-15 23:22 UTC #0 lore raw

Hello,

CIFS cleanup_volume_info_contents() looks like having a memory corruption problem.
When UNCip is set to "&vol->UNC[2]" in cifs_parse_mount_options(), it should not be kfree()-ed in cleanup_volume_info_contents().

If it is correct and the code in mainline is not fixed yet, then here is a patch.

fs/cifs/connect.c +2 -1

index ccc1afa..e0ea721 100644
@@ -2838,7 +2838,8 @@ cleanup_volume_info_contents(struct smb_vol *volume_info)
 	kfree(volume_info->username);
 	kzfree(volume_info->password);
 	kfree(volume_info->UNC);
-	kfree(volume_info->UNCip);
+	if (volume_info->UNCip != volume_info->UNC + 2)
+		kfree(volume_info->UNCip);
 	kfree(volume_info->domainname);
 	kfree(volume_info->iocharset);
 	kfree(volume_info->prepath);

J. R. Okajima

Jeff Layton

Reviewed-by

2011-08-18 01:26 UTC #1 lore raw

Quoted 18 lines · On Tue, 16 Aug 2011 08:22:17 +0900 "J. R. Okajima" <[email protected]>…

Hello,
CIFS cleanup_volume_info_contents() looks like having a memory corruption problem.
When UNCip is set to "&vol->UNC[2]" in cifs_parse_mount_options(), it should not be kfree()-ed in cleanup_volume_info_contents().
If it is correct and the code in mainline is not fixed yet, then here is a patch.
fs/cifs/connect.c +0 -0
index ccc1afa..e0ea721 100644

fs/cifs/connect.c cleanup_volume_info_contents(struct smb_vol *volume_info) · line 2845

Quoted 9 lines

fs/cifs/connect.c +2 -1

@@ -2838,7 +2838,8 @@ cleanup_volume_info_contents(struct smb_vol *volume_info)
 	kfree(volume_info->username);
 	kzfree(volume_info->password);
 	kfree(volume_info->UNC);
-	kfree(volume_info->UNCip);
+	if (volume_info->UNCip != volume_info->UNC + 2)
+		kfree(volume_info->UNCip);
 	kfree(volume_info->domainname);
 	kfree(volume_info->iocharset);
 	kfree(volume_info->prepath);

Looks correct, nice catch.

Reviewed-by Jeff Layton <[email protected]>

Jeff Layton

2011-08-18 10:39 UTC #2 lore raw

In reply to J. R. Okajima

Quoted 18 lines · On Tue, 16 Aug 2011 08:22:17 +0900 "J. R. Okajima" <[email protected]>…

Hello,
CIFS cleanup_volume_info_contents() looks like having a memory corruption problem.
When UNCip is set to "&vol->UNC[2]" in cifs_parse_mount_options(), it should not be kfree()-ed in cleanup_volume_info_contents().
If it is correct and the code in mainline is not fixed yet, then here is a patch.
fs/cifs/connect.c +0 -0
index ccc1afa..e0ea721 100644

fs/cifs/connect.c cleanup_volume_info_contents(struct smb_vol *volume_info) · line 2845

Quoted 9 lines

fs/cifs/connect.c +2 -1

@@ -2838,7 +2838,8 @@ cleanup_volume_info_contents(struct smb_vol *volume_info)
 	kfree(volume_info->username);
 	kzfree(volume_info->password);
 	kfree(volume_info->UNC);
-	kfree(volume_info->UNCip);
+	if (volume_info->UNCip != volume_info->UNC + 2)
+		kfree(volume_info->UNCip);
 	kfree(volume_info->domainname);
 	kfree(volume_info->iocharset);
 	kfree(volume_info->prepath);

Also, this patch should obviously go to stable too.

Signature · 1 line

Jeff Layton <[email protected]>

Steve French

2011-08-18 16:55 UTC #3 lore raw

In reply to J. R. Okajima

merged

Quoted 29 lines · J. R. Okajima

Hello,
CIFS cleanup_volume_info_contents() looks like having a memory corruption problem.
When UNCip is set to "&vol->UNC[2]" in cifs_parse_mount_options(), it should not be kfree()-ed in cleanup_volume_info_contents().
If it is correct and the code in mainline is not fixed yet, then here is a patch.
fs/cifs/connect.c +2 -1
index ccc1afa..e0ea721 100644
@@ -2838,7 +2838,8 @@ cleanup_volume_info_contents(struct smb_vol *volume_info)
        kfree(volume_info->username);
        kzfree(volume_info->password);
        kfree(volume_info->UNC);
-       kfree(volume_info->UNCip);
+       if (volume_info->UNCip != volume_info->UNC + 2)
+               kfree(volume_info->UNCip);
        kfree(volume_info->domainname);
        kfree(volume_info->iocharset);
        kfree(volume_info->prepath);
J. R. Okajima

Signature · 3 lines

Thanks,

Steve