Skip to contentSkip to conversation

(resend) Fix potential kfree on vmalloc-ed area of request_sock_queue

PATCH In review Pavel Emelyanov 3 participants 3 messages Nov 15, 2007

Subsystems (3)
Networking Network drivers Sockets
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

PATCH 1/2 (resend) Fix potential kfree on vmalloc-ed area of request_sock_queue

The request_sock_queue's listen_opt is either vmalloc-ed or kmalloc-ed depending on the number of table entries. Thus it is expected to be handled properly on free, which is done in the reqsk_queue_destroy().

However the error path in inet_csk_listen_start() calls the lite version of reqsk_queue_destroy, called __reqsk_queue_destroy, which calls the kfree unconditionally.

Fix this and move the __reqsk_queue_destroy into a .c file as it looks too big to be inline.

As David also noticed, this is an error recovery path only, so no locking is required and the lopt is known to be not NULL.

Signed-off-by Pavel Emelyanov <[email protected]>

---

include/net/request_sock.h +1 -5
index 7aed02c..0a954ee 100644
@@ -136,11 +136,7 @@ static inline struct listen_sock *reqsk_queue_yank_listen_sk(struct request_sock
 	return lopt;
 }
 
-static inline void __reqsk_queue_destroy(struct request_sock_queue *queue)
-{
-	kfree(reqsk_queue_yank_listen_sk(queue));
-}
-
+extern void __reqsk_queue_destroy(struct request_sock_queue *queue);
 extern void reqsk_queue_destroy(struct request_sock_queue *queue);
 
 static inline struct request_sock *
net/core/request_sock.c +22 -0
index 5f0818d..dd78b85 100644
@@ -71,6 +71,28 @@ int reqsk_queue_alloc(struct request_sock_queue *queue,
 
 EXPORT_SYMBOL(reqsk_queue_alloc);
 
+void __reqsk_queue_destroy(struct request_sock_queue *queue)
+{
+	struct listen_sock *lopt;
+	size_t lopt_size;
+
+	/*
+	 * this is an error recovery path only
+	 * no locking needed and the lopt is not NULL
+	 */
+
+	lopt = queue->listen_opt;
+	lopt_size = sizeof(struct listen_sock) +
+		lopt->nr_table_entries * sizeof(struct request_sock *);
+
+	if (lopt_size > PAGE_SIZE)
+		vfree(lopt);
+	else
+		kfree(lopt);
+}
+
+EXPORT_SYMBOL(__reqsk_queue_destroy);
+
 void reqsk_queue_destroy(struct request_sock_queue *queue)
 {
 	/* make all the listen_opt local to us */
Signature · 1 line
1.5.3.4
Acked-by on v1 1/2
#1 lore raw
Quoted 9 lines · On Thu, 15 Nov 2007 11:41:37 +0300 Pavel Emelyanov <[email protected]> wrote:

The request_sock_queue's listen_opt is either vmalloc-ed or kmalloc-ed depending on the number of table entries. Thus it is expected to be handled properly on free, which is done in the reqsk_queue_destroy().

However the error path in inet_csk_listen_start() calls the lite version of reqsk_queue_destroy, called __reqsk_queue_destroy, which calls the kfree unconditionally.

Quoted 8 lines

Fix this and move the __reqsk_queue_destroy into a .c file as it looks too big to be inline.

As David also noticed, this is an error recovery path only, so no locking is required and the lopt is known to be not NULL.

Signed-off-by Pavel Emelyanov <[email protected]>
Acked-by Eric Dumazet <[email protected]>

Thank you for finding this bug Pavel

maintainer
#2 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 15 Nov 2007 10:21:01 +0100

Quoted 18 lines
Quoted 15 lines · On Thu, 15 Nov 2007 11:41:37 +0300 Pavel Emelyanov <[email protected]> wrote:

The request_sock_queue's listen_opt is either vmalloc-ed or kmalloc-ed depending on the number of table entries. Thus it is expected to be handled properly on free, which is done in the reqsk_queue_destroy().

However the error path in inet_csk_listen_start() calls the lite version of reqsk_queue_destroy, called __reqsk_queue_destroy, which calls the kfree unconditionally.

Fix this and move the __reqsk_queue_destroy into a .c file as it looks too big to be inline.

As David also noticed, this is an error recovery path only, so no locking is required and the lopt is known to be not NULL.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Pavel Emelyanov <[email protected]>
Acked-by Eric Dumazet <[email protected]>

Thank you for finding this bug Pavel

Indeed.

I applied this, but what I did was I combined both changes into one because to me they logically belong together.

Thanks again Pavel!