smc: some potential use after free bugs

PATCH In review Dan Carpenter 3 participants 5 messages Jan 26, 2017 – Jan 27, 2017

Subsystems (5)

Networking Network drivers Kernel janitors s390 +1 more subsystems

lore

Share & tools

lore

Dan Carpenter

author

2017-01-26 09:05 UTC #0 lore raw

PATCH 1/1 smc: some potential use after free bugs

Say we got really unlucky and these failed on the last iteration, then it could lead to a use after free bug.

Fixes cd6851f30386 ("smc: remote memory buffers (RMBs)")

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

net/smc/smc_core.c +5 -0

index 8b1d34378829..941279e1504e 100644
@@ -535,6 +535,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 			/* if send buffer allocation has failed,
 			 * try a smaller one
 			 */
+			sndbuf_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -543,6 +544,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(sndbuf_desc->cpu_addr);
 			kfree(sndbuf_desc);
+			sndbuf_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		sndbuf_desc->used = 1;

@@ -599,6 +601,7 @@ int smc_rmb_create(struct smc_sock *smc)
 			/* if RMB allocation has failed,
 			 * try a smaller one
 			 */
+			rmb_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -607,6 +610,7 @@ int smc_rmb_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,

@@ -619,6 +623,7 @@ int smc_rmb_create(struct smc_sock *smc)
 					 DMA_FROM_DEVICE);
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue;
 		}
 		rmb_desc->used = 1;

Ursula Braun

2017-01-26 11:23 UTC #1 lore raw

Quoted 2 lines · Dan Carpenter

Say we got really unlucky and these failed on the last iteration, then it could lead to a use after free bug.

thanks for reporting this! I had already a similar patch prepared, but not yet submitted. It contains all your added lines plus these additional pre-initializations at definition time:

@@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)

        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *sndbuf_desc;
+ struct smc_buf_desc *sndbuf_desc = NULL;
        int rc;
 
        /* use socket send buffer size (w/o overhead) as start value */

@@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)

        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *rmb_desc;
+ struct smc_buf_desc *rmb_desc = NULL;
        int rc;
 
        /* use socket recv buffer size (w/o overhead) as start value */

If you do not contradict, I will post my enhanced patch version.

Quoted 49 lines

Fixes cd6851f30386 ("smc: remote memory buffers (RMBs)")

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

net/smc/smc_core.c +5 -0

index 8b1d34378829..941279e1504e 100644
@@ -535,6 +535,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 			/* if send buffer allocation has failed,
 			 * try a smaller one
 			 */
+			sndbuf_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -543,6 +544,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(sndbuf_desc->cpu_addr);
 			kfree(sndbuf_desc);
+			sndbuf_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		sndbuf_desc->used = 1;

@@ -599,6 +601,7 @@ int smc_rmb_create(struct smc_sock *smc)
 			/* if RMB allocation has failed,
 			 * try a smaller one
 			 */
+			rmb_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -607,6 +610,7 @@ int smc_rmb_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,

@@ -619,6 +623,7 @@ int smc_rmb_create(struct smc_sock *smc)
 					 DMA_FROM_DEVICE);
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue;
 		}
 		rmb_desc->used = 1;

Dan Carpenter

author

2017-01-26 11:32 UTC #2 lore raw

Quoted 28 lines · Ursula Braun

Quoted 2 lines · Dan Carpenter

Say we got really unlucky and these failed on the last iteration, then it could lead to a use after free bug.

thanks for reporting this! I had already a similar patch prepared, but not yet submitted. It contains all your added lines plus these additional pre-initializations at definition time:

(quoted diff) +2 -2

@@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *sndbuf_desc;
+ struct smc_buf_desc *sndbuf_desc = NULL;
        int rc;
 
        /* use socket send buffer size (w/o overhead) as start value */

@@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)
        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *rmb_desc;
+ struct smc_buf_desc *rmb_desc = NULL;
        int rc;
 
        /* use socket recv buffer size (w/o overhead) as start value */

Quoted 1 line

If you do not contradict, I will post my enhanced patch version.

Those are obviously harmless changes...

My static checker knows those aren't required so it doesn't complain about uninitialized variables. But not all static analysis tools are as clever. ;)

regards,
dan carpenter

walter harms

2017-01-26 13:02 UTC #3 lore raw

In reply to Dan Carpenter

Am 26.01.2017 12:23, schrieb Ursula Braun:

Quoted 28 lines · Dan Carpenter

Quoted 2 lines

Say we got really unlucky and these failed on the last iteration, then it could lead to a use after free bug.

thanks for reporting this! I had already a similar patch prepared, but not yet submitted. It contains all your added lines plus these additional pre-initializations at definition time:

(quoted diff) +2 -2

@@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *sndbuf_desc;
+ struct smc_buf_desc *sndbuf_desc = NULL;
        int rc;
 
        /* use socket send buffer size (w/o overhead) as start value */

@@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)
        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *rmb_desc;
+ struct smc_buf_desc *rmb_desc = NULL;
        int rc;
 
        /* use socket recv buffer size (w/o overhead) as start value */

Quoted 1 line

If you do not contradict, I will post my enhanced patch version.

Independend of that ...

would it be possible to make sndbuf_desc->cpu_addr a zero size array and then simply do a:
sndbuf_desc = kzalloc(sizeof(*sndbuf_desc)+tmp_bufsize, GFP_KERNEL);

(read also: is the whole list of __GPF* really needed ?)

that would make the memhandling more easy

re,
wh

Quoted 55 lines

Quoted 42 lines

Fixes cd6851f30386 ("smc: remote memory buffers (RMBs)")

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

net/smc/smc_core.c +4 -0

index 8b1d34378829..941279e1504e 100644
@@ -535,6 +535,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 			/* if send buffer allocation has failed,
 			 * try a smaller one
 			 */
+			sndbuf_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -543,6 +544,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(sndbuf_desc->cpu_addr);
 			kfree(sndbuf_desc);
+			sndbuf_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		sndbuf_desc->used = 1;

@@ -599,6 +601,7 @@ int smc_rmb_create(struct smc_sock *smc)
 			/* if RMB allocation has failed,
 			 * try a smaller one
 			 */
+			rmb_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -607,6 +610,7 @@ int smc_rmb_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,

net/smc/smc_core.c int smc_rmb_create(struct smc_sock *smc) · line 629

Quoted 7 lines

net/smc/smc_core.c +1 -0

@@ -619,6 +623,7 @@ int smc_rmb_create(struct smc_sock *smc)
 					 DMA_FROM_DEVICE);
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue;
 		}
 		rmb_desc->used = 1;

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ursula Braun

2017-01-27 10:13 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 31 lines · walter harms

Am 26.01.2017 12:23, schrieb Ursula Braun:

Quoted 28 lines · Dan Carpenter

Quoted 2 lines

Say we got really unlucky and these failed on the last iteration, then it could lead to a use after free bug.

thanks for reporting this! I had already a similar patch prepared, but not yet submitted. It contains all your added lines plus these additional pre-initializations at definition time:

(quoted diff) +2 -2

@@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *sndbuf_desc;
+ struct smc_buf_desc *sndbuf_desc = NULL;
        int rc;
 
        /* use socket send buffer size (w/o overhead) as start value */

@@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)
        struct smc_connection *conn = &smc->conn;
        struct smc_link_group *lgr = conn->lgr;
        int tmp_bufsize, tmp_bufsize_short;
-   struct smc_buf_desc *rmb_desc;
+ struct smc_buf_desc *rmb_desc = NULL;
        int rc;
 
        /* use socket recv buffer size (w/o overhead) as start value */

Quoted 9 lines

Quoted 1 line
If you do not contradict, I will post my enhanced patch version.
Independend of that ...
would it be possible to make sndbuf_desc->cpu_addr a zero size array and then simply do a:
sndbuf_desc = kzalloc(sizeof(*sndbuf_desc)+tmp_bufsize, GFP_KERNEL);

The idea had been to keep DMA-mapping memory separate from other memory. But I will experiment with your idea.

Quoted 1 line

(read also: is the whole list of __GPF* really needed ?)

Currently SMC-R tries to get large chunks of real consecutive memory initially and lowers this requirement if not available. We have future work in mind to implement a scattered solution, but for now we accept initially failing memory allocations, and do not want these sometimes likely failings to be noisy.

Quoted 63 lines

that would make the memhandling more easy

re,
wh

Quoted 55 lines

Quoted 42 lines

Fixes cd6851f30386 ("smc: remote memory buffers (RMBs)")

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

net/smc/smc_core.c +4 -0

index 8b1d34378829..941279e1504e 100644
@@ -535,6 +535,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 			/* if send buffer allocation has failed,
 			 * try a smaller one
 			 */
+			sndbuf_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -543,6 +544,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(sndbuf_desc->cpu_addr);
 			kfree(sndbuf_desc);
+			sndbuf_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		sndbuf_desc->used = 1;

@@ -599,6 +601,7 @@ int smc_rmb_create(struct smc_sock *smc)
 			/* if RMB allocation has failed,
 			 * try a smaller one
 			 */
+			rmb_desc = NULL;
 			continue;
 		}
 		rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,

@@ -607,6 +610,7 @@ int smc_rmb_create(struct smc_sock *smc)
 		if (rc) {
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
 		rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,

net/smc/smc_core.c int smc_rmb_create(struct smc_sock *smc) · line 629

Quoted 7 lines

net/smc/smc_core.c +1 -0

@@ -619,6 +623,7 @@ int smc_rmb_create(struct smc_sock *smc)
 					 DMA_FROM_DEVICE);
 			kfree(rmb_desc->cpu_addr);
 			kfree(rmb_desc);
+			rmb_desc = NULL;
 			continue;
 		}
 		rmb_desc->used = 1;

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html