Skip to contentSkip to conversation

staging: ramster: fix range checks in zcache_autocreate_pool()

PATCH In review Dan Carpenter 3 participants 5 messages Sep 6, 2012

Subsystems (3)
Memory management Staging Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 staging: ramster: fix range checks in zcache_autocreate_pool()

If "pool_id" is negative then it leads to a read before the start of the array. If "cli_id" is out of bounds then it leads to a NULL dereference of "cli". GCC would have warned about that bug except that we initialized the warning message away.

Also it's better to put the parameter names into the function declaration in the .h file. It serves as a kind of documentation.

Signed-off-by Dan Carpenter <[email protected]>

---
BTW, This file has a ton of GCC warnings. This function returns -1 on error which is a nonsense return code but the return value is not checked anyway. *Grumble*.

drivers/staging/ramster/zcache.h +1 -1
index c59666e..81722b3 100644
@@ -42,7 +42,7 @@ extern void zcache_decompress_to_page(char *, unsigned int, struct page *);
 #ifdef CONFIG_RAMSTER
 extern void *zcache_pampd_create(char *, unsigned int, bool, int,
 				struct tmem_handle *);
-extern int zcache_autocreate_pool(int, int, bool);
+int zcache_autocreate_pool(unsigned int cli_id, unsigned int pool_id, bool eph);
 #endif
 
 #define MAX_POOLS_PER_CLIENT 16
drivers/staging/ramster/zcache-main.c +6 -4
index 24b3d4a..86e19d6 100644
@@ -1338,10 +1338,10 @@ static int zcache_local_new_pool(uint32_t flags)
 	return zcache_new_pool(LOCAL_CLIENT, flags);
 }
 
-int zcache_autocreate_pool(int cli_id, int pool_id, bool eph)
+int zcache_autocreate_pool(unsigned int cli_id, unsigned int pool_id, bool eph)
 {
 	struct tmem_pool *pool;
-	struct zcache_client *cli = NULL;
+	struct zcache_client *cli;
 	uint32_t flags = eph ? 0 : TMEM_POOL_PERSIST;
 	int ret = -1;
 

@@ -1350,8 +1350,10 @@ int zcache_autocreate_pool(int cli_id, int pool_id, bool eph)
 		goto out;
 	if (pool_id >= MAX_POOLS_PER_CLIENT)
 		goto out;
-	else if ((unsigned int)cli_id < MAX_CLIENTS)
-		cli = &zcache_clients[cli_id];
+	if (cli_id >= MAX_CLIENTS)
+		goto out;
+
+	cli = &zcache_clients[cli_id];
 	if ((eph && disable_cleancache) || (!eph && disable_frontswap)) {
 		pr_err("zcache_autocreate_pool: pool type disabled\n");
 		goto out;
maintainer
#1 lore raw
Quoted 5 lines · Dan Carpenter

If "pool_id" is negative then it leads to a read before the start of the array. If "cli_id" is out of bounds then it leads to a NULL dereference of "cli". GCC would have warned about that bug except that we initialized the warning message away.

Quoted 8 lines

Also it's better to put the parameter names into the function declaration in the .h file. It serves as a kind of documentation.

Signed-off-by Dan Carpenter <[email protected]>

---
BTW, This file has a ton of GCC warnings. This function returns -1 on error which is a nonsense return code but the return value is not checked anyway. *Grumble*.

I agree, it's very messy. Dan Magenheimer should have known better, and he better be sending me a patch soon to remove these warnings (hint...)

thanks,

greg k-h

#2 lore raw
Quoted 12 lines

From: Greg Kroah-Hartman [mailto:[email protected]]
Subject: Re: [patch] staging: ramster: fix range checks in zcache_autocreate_pool()

Quoted 8 lines · Dan Carpenter

If "pool_id" is negative then it leads to a read before the start of the array. If "cli_id" is out of bounds then it leads to a NULL dereference of "cli". GCC would have warned about that bug except that we initialized the warning message away.

Also it's better to put the parameter names into the function declaration in the .h file. It serves as a kind of documentation.

Quoted 8 lines
Quoted 5 lines
Signed-off-by Dan Carpenter <[email protected]>

---
BTW, This file has a ton of GCC warnings. This function returns -1 on error which is a nonsense return code but the return value is not checked anyway. *Grumble*.

I agree, it's very messy. Dan Magenheimer should have known better, and he better be sending me a patch soon to remove these warnings (hint...)

On its way soon.

Dan

#3 lore raw
Quoted 21 lines

From: Dan Magenheimer
Subject: RE: [patch] staging: ramster: fix range checks in zcache_autocreate_pool()

Quoted 18 lines

From: Greg Kroah-Hartman [mailto:[email protected]]
Subject: Re: [patch] staging: ramster: fix range checks in zcache_autocreate_pool()

Quoted 13 lines · Dan Carpenter

If "pool_id" is negative then it leads to a read before the start of the array. If "cli_id" is out of bounds then it leads to a NULL dereference of "cli". GCC would have warned about that bug except that we initialized the warning message away.

Also it's better to put the parameter names into the function declaration in the .h file. It serves as a kind of documentation.

Signed-off-by Dan Carpenter <[email protected]>

---
BTW, This file has a ton of GCC warnings. This function returns -1 on error which is a nonsense return code but the return value is not checked anyway. *Grumble*.

Quoted 6 lines
Quoted 2 lines

I agree, it's very messy. Dan Magenheimer should have known better, and he better be sending me a patch soon to remove these warnings (hint...)

On its way soon.

Quoted 1 line
Quoted 1 line

BTW, This file has a ton of GCC warnings.

Submitted (with typo in kernel-janitors address)... but I also just realized from previous feedback on a much earlier thread...

I use a stable RHEL6-ish system for devel/test with gcc-4.4.5, and newer gcc's may report more warnings than I see or have fixed.

If there is now a required newer gcc version for patch submittals, please let me know.

(However, I will be away from email for a few days, so apologies in advance if I can't respond immediately.)

Acked-by on v1 1/1
#4 lore raw
In reply to Dan Carpenter
Quoted 12 lines

From: Dan Carpenter
Sent: Thursday, September 06, 2012 6:40 AM
To: Greg Kroah-Hartman
Cc: Dan Magenheimer; Konrad Rzeszutek Wilk; [email protected]; [email protected]; kernel- [email protected]
Subject: [patch] staging: ramster: fix range checks in zcache_autocreate_pool()

If "pool_id" is negative then it leads to a read before the start of the array. If "cli_id" is out of bounds then it leads to a NULL dereference of "cli". GCC would have warned about that bug except that we initialized the warning message away.

Quoted 4 lines

Also it's better to put the parameter names into the function declaration in the .h file. It serves as a kind of documentation.

Signed-off-by Dan Carpenter <[email protected]>
Acked-by Dan Magenheimer <[email protected]>
Self-flagellated-by Dan Magenheimer <[email protected]>
Quoted 48 lines

---
BTW, This file has a ton of GCC warnings. This function returns -1 on error which is a nonsense return code but the return value is not checked anyway. *Grumble*.

drivers/staging/ramster/zcache.h +1 -1
index c59666e..81722b3 100644
@@ -42,7 +42,7 @@ extern void zcache_decompress_to_page(char *, unsigned int, struct page *);
 #ifdef CONFIG_RAMSTER
 extern void *zcache_pampd_create(char *, unsigned int, bool, int,
 				struct tmem_handle *);
-extern int zcache_autocreate_pool(int, int, bool);
+int zcache_autocreate_pool(unsigned int cli_id, unsigned int pool_id, bool eph);
 #endif
 
 #define MAX_POOLS_PER_CLIENT 16
drivers/staging/ramster/zcache-main.c +6 -4
index 24b3d4a..86e19d6 100644
@@ -1338,10 +1338,10 @@ static int zcache_local_new_pool(uint32_t flags)
 	return zcache_new_pool(LOCAL_CLIENT, flags);
 }
 
-int zcache_autocreate_pool(int cli_id, int pool_id, bool eph)
+int zcache_autocreate_pool(unsigned int cli_id, unsigned int pool_id, bool eph)
 {
 	struct tmem_pool *pool;
-	struct zcache_client *cli = NULL;
+	struct zcache_client *cli;
 	uint32_t flags = eph ? 0 : TMEM_POOL_PERSIST;
 	int ret = -1;
 

@@ -1350,8 +1350,10 @@ int zcache_autocreate_pool(int cli_id, int pool_id, bool eph)
 		goto out;
 	if (pool_id >= MAX_POOLS_PER_CLIENT)
 		goto out;
-	else if ((unsigned int)cli_id < MAX_CLIENTS)
-		cli = &zcache_clients[cli_id];
+	if (cli_id >= MAX_CLIENTS)
+		goto out;
+
+	cli = &zcache_clients[cli_id];
 	if ((eph && disable_cleancache) || (!eph && disable_frontswap)) {
 		pr_err("zcache_autocreate_pool: pool type disabled\n");
 		goto out;