[PATCH] packet: fix leakage of tx_ring memory

[PATCH] packet: fix leakage of tx_ring memory

[Phil Sutter]

When releasing a packet socket, the routine packet_set_ring() is reused
to free rings instead of allocating them. But when calling it for the
first time, it fills req->tp_block_nr with the value of rb->pg_vec_len
which in the second invocation makes it bail out since req->tp_block_nr
is greater zero but req->tp_block_size is zero.

This patch solves the problem by passing a zeroed auto-variable to
packet_set_ring() upon each invocation from packet_release().

As far as I can tell, this issue exists even since 69e3c75 (net: TX_RING
and packet mmap), i.e. the original inclusion of TX ring support into
af_packet, but applies only to sockets with both RX and TX ring
allocated, which is probably why this was unnoticed all the time.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
Cc: Johann Baudy <johann.baudy@gnu-log.net>
Cc: stable@kernel.org
---
 net/packet/af_packet.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a91fd0b..d5f519a 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2328,6 +2328,13 @@ static int packet_sendmsg(struct kiocb *iocb, struct socket *sock,
 	return rc;
 }
 
+static int packet_free_ring(struct sock *sk, int tx_ring)
+{
+	union tpacket_req_u req_u = { 0 };
+
+	return packet_set_ring(sk, &req_u, 1, tx_ring);
+}
+
 /*
  *	Close a PACKET socket. This is fairly simple. We immediately go
  *	to 'closed' state and remove our protocol entry in the device list.
@@ -2338,7 +2345,6 @@ static int packet_release(struct socket *sock)
 	struct sock *sk = sock->sk;
 	struct packet_sock *po;
 	struct net *net;
-	union tpacket_req_u req_u;
 
 	if (!sk)
 		return 0;
@@ -2364,13 +2370,11 @@ static int packet_release(struct socket *sock)
 
 	packet_flush_mclist(sk);
 
-	memset(&req_u, 0, sizeof(req_u));
-
 	if (po->rx_ring.pg_vec)
-		packet_set_ring(sk, &req_u, 1, 0);
+		packet_free_ring(sk, 0);
 
 	if (po->tx_ring.pg_vec)
-		packet_set_ring(sk, &req_u, 1, 1);
+		packet_free_ring(sk, 1);
 
 	fanout_release(sk);
 
-- 
1.7.12.3

Re: [PATCH] packet: fix leakage of tx_ring memory

[Daniel Borkmann]

On 02/01/2013 02:57 PM, Phil Sutter wrote:
> When releasing a packet socket, the routine packet_set_ring() is reused
> to free rings instead of allocating them. But when calling it for the
> first time, it fills req->tp_block_nr with the value of rb->pg_vec_len
> which in the second invocation makes it bail out since req->tp_block_nr
> is greater zero but req->tp_block_size is zero.
>
> This patch solves the problem by passing a zeroed auto-variable to
> packet_set_ring() upon each invocation from packet_release().
>
> As far as I can tell, this issue exists even since 69e3c75 (net: TX_RING
> and packet mmap), i.e. the original inclusion of TX ring support into
> af_packet, but applies only to sockets with both RX and TX ring
> allocated, which is probably why this was unnoticed all the time.
>
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> Cc: Johann Baudy <johann.baudy@gnu-log.net>
> Cc: stable@kernel.org
> ---

[...]

> +static int packet_free_ring(struct sock *sk, int tx_ring)
> +{
> +	union tpacket_req_u req_u = { 0 };
> +
> +	return packet_set_ring(sk, &req_u, 1, tx_ring);
> +}
> +
>   /*
>    *	Close a PACKET socket. This is fairly simple. We immediately go
>    *	to 'closed' state and remove our protocol entry in the device list.
> @@ -2338,7 +2345,6 @@ static int packet_release(struct socket *sock)
>   	struct sock *sk = sock->sk;
>   	struct packet_sock *po;
>   	struct net *net;
> -	union tpacket_req_u req_u;
>
>   	if (!sk)
>   		return 0;
> @@ -2364,13 +2370,11 @@ static int packet_release(struct socket *sock)
>
>   	packet_flush_mclist(sk);
>
> -	memset(&req_u, 0, sizeof(req_u));
> -
>   	if (po->rx_ring.pg_vec)
> -		packet_set_ring(sk, &req_u, 1, 0);
> +		packet_free_ring(sk, 0);
>
>   	if (po->tx_ring.pg_vec)
> -		packet_set_ring(sk, &req_u, 1, 1);
> +		packet_free_ring(sk, 1);

Good catch!

Nitpicking:

I think it would be easier / more readable to simply move the memset into
the two ifs than introducing an extra function for just doing that.

(Also don't cc stable, since David is deciding about this anyway.)

Re: [PATCH] packet: fix leakage of tx_ring memory

[Phil Sutter]

On Fri, Feb 01, 2013 at 05:05:08PM +0100, Daniel Borkmann wrote:
> On 02/01/2013 02:57 PM, Phil Sutter wrote:
> > When releasing a packet socket, the routine packet_set_ring() is reused
> > to free rings instead of allocating them. But when calling it for the
> > first time, it fills req->tp_block_nr with the value of rb->pg_vec_len
> > which in the second invocation makes it bail out since req->tp_block_nr
> > is greater zero but req->tp_block_size is zero.
> >
> > This patch solves the problem by passing a zeroed auto-variable to
> > packet_set_ring() upon each invocation from packet_release().
> >
> > As far as I can tell, this issue exists even since 69e3c75 (net: TX_RING
> > and packet mmap), i.e. the original inclusion of TX ring support into
> > af_packet, but applies only to sockets with both RX and TX ring
> > allocated, which is probably why this was unnoticed all the time.
> >
> > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > Cc: Johann Baudy <johann.baudy@gnu-log.net>
> > Cc: stable@kernel.org
> > ---
> 
> [...]
> 
> > +static int packet_free_ring(struct sock *sk, int tx_ring)
> > +{
> > +	union tpacket_req_u req_u = { 0 };
> > +
> > +	return packet_set_ring(sk, &req_u, 1, tx_ring);
> > +}
> > +
> >   /*
> >    *	Close a PACKET socket. This is fairly simple. We immediately go
> >    *	to 'closed' state and remove our protocol entry in the device list.
> > @@ -2338,7 +2345,6 @@ static int packet_release(struct socket *sock)
> >   	struct sock *sk = sock->sk;
> >   	struct packet_sock *po;
> >   	struct net *net;
> > -	union tpacket_req_u req_u;
> >
> >   	if (!sk)
> >   		return 0;
> > @@ -2364,13 +2370,11 @@ static int packet_release(struct socket *sock)
> >
> >   	packet_flush_mclist(sk);
> >
> > -	memset(&req_u, 0, sizeof(req_u));
> > -
> >   	if (po->rx_ring.pg_vec)
> > -		packet_set_ring(sk, &req_u, 1, 0);
> > +		packet_free_ring(sk, 0);
> >
> >   	if (po->tx_ring.pg_vec)
> > -		packet_set_ring(sk, &req_u, 1, 1);
> > +		packet_free_ring(sk, 1);
> 
> Good catch!
> 
> Nitpicking:
> 
> I think it would be easier / more readable to simply move the memset into
> the two ifs than introducing an extra function for just doing that.

Yes, this was just how my fix looked like initially, but I didn't like
the resulting code duplication. Indeed, the extra function adds another
point of code flow redirection. On the other hand, it implicitly points
out that basically the same is done for both rings.

In my point of view, both ways are equally acceptable. If you prefer the
other one for mainline inclusion, just let me know and I submit an
appropriate patch.

> (Also don't cc stable, since David is deciding about this anyway.)

Oh, OK. Thanks for the hint. Every time I wonder how to do this the
right way, seems I haven't found the correct documentation for it yet.
Moreover, stable@kernel.org isn't even valid. I shouldn't have trusted
google (http://www.kernel.org/doc/Documentation/stable_kernel_rules.txt)
over my local clones. ;)


Best wishes,

Phil Sutter
Software Engineer

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter@viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Geschäftsführer: Simon Kissel

Re: [PATCH] packet: fix leakage of tx_ring memory

[Daniel Borkmann]

On 02/01/2013 05:21 PM, Phil Sutter wrote:
> On Fri, Feb 01, 2013 at 05:05:08PM +0100, Daniel Borkmann wrote:

>> I think it would be easier / more readable to simply move the memset into
>> the two ifs than introducing an extra function for just doing that.
>
> Yes, this was just how my fix looked like initially, but I didn't like
> the resulting code duplication. Indeed, the extra function adds another
> point of code flow redirection. On the other hand, it implicitly points
> out that basically the same is done for both rings.
>
> In my point of view, both ways are equally acceptable. If you prefer the
> other one for mainline inclusion, just let me know and I submit an
> appropriate patch.

Yes, that'd be good, thanks.

[PATCH v2] packet: fix leakage of tx_ring memory

[Phil Sutter]

When releasing a packet socket, the routine packet_set_ring() is reused
to free rings instead of allocating them. But when calling it for the
first time, it fills req->tp_block_nr with the value of rb->pg_vec_len
which in the second invocation makes it bail out since req->tp_block_nr
is greater zero but req->tp_block_size is zero.

This patch solves the problem by passing a zeroed auto-variable to
packet_set_ring() upon each invocation from packet_release().

As far as I can tell, this issue exists even since 69e3c75 (net: TX_RING
and packet mmap), i.e. the original inclusion of TX ring support into
af_packet, but applies only to sockets with both RX and TX ring
allocated, which is probably why this was unnoticed all the time.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
Cc: Johann Baudy <johann.baudy@gnu-log.net>
Cc: Daniel Borkmann <dborkman@redhat.com>

---
Changes since v1:
- less functions
- more code duplication
---
 net/packet/af_packet.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a91fd0b..07c9483 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2364,13 +2364,15 @@ static int packet_release(struct socket *sock)
 
 	packet_flush_mclist(sk);
 
-	memset(&req_u, 0, sizeof(req_u));
-
-	if (po->rx_ring.pg_vec)
+	if (po->rx_ring.pg_vec) {
+		memset(&req_u, 0, sizeof(req_u));
 		packet_set_ring(sk, &req_u, 1, 0);
+	}
 
-	if (po->tx_ring.pg_vec)
+	if (po->tx_ring.pg_vec) {
+		memset(&req_u, 0, sizeof(req_u));
 		packet_set_ring(sk, &req_u, 1, 1);
+	}
 
 	fanout_release(sk);
 
-- 
1.7.12.3

Re: [PATCH v2] packet: fix leakage of tx_ring memory

[Daniel Borkmann]

On 02/01/2013 06:21 PM, Phil Sutter wrote:
> When releasing a packet socket, the routine packet_set_ring() is reused
> to free rings instead of allocating them. But when calling it for the
> first time, it fills req->tp_block_nr with the value of rb->pg_vec_len
> which in the second invocation makes it bail out since req->tp_block_nr
> is greater zero but req->tp_block_size is zero.
>
> This patch solves the problem by passing a zeroed auto-variable to
> packet_set_ring() upon each invocation from packet_release().
>
> As far as I can tell, this issue exists even since 69e3c75 (net: TX_RING
> and packet mmap), i.e. the original inclusion of TX ring support into
> af_packet, but applies only to sockets with both RX and TX ring
> allocated, which is probably why this was unnoticed all the time.
>
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> Cc: Johann Baudy <johann.baudy@gnu-log.net>
> Cc: Daniel Borkmann <dborkman@redhat.com>

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH v2] packet: fix leakage of tx_ring memory

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Fri, 01 Feb 2013 18:25:13 +0100

> On 02/01/2013 06:21 PM, Phil Sutter wrote:
>> When releasing a packet socket, the routine packet_set_ring() is
>> reused
>> to free rings instead of allocating them. But when calling it for the
>> first time, it fills req->tp_block_nr with the value of rb->pg_vec_len
>> which in the second invocation makes it bail out since
>> req->tp_block_nr
>> is greater zero but req->tp_block_size is zero.
>>
>> This patch solves the problem by passing a zeroed auto-variable to
>> packet_set_ring() upon each invocation from packet_release().
>>
>> As far as I can tell, this issue exists even since 69e3c75 (net:
>> TX_RING
>> and packet mmap), i.e. the original inclusion of TX ring support into
>> af_packet, but applies only to sockets with both RX and TX ring
>> allocated, which is probably why this was unnoticed all the time.
>>
>> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
>> Cc: Johann Baudy <johann.baudy@gnu-log.net>
>> Cc: Daniel Borkmann <dborkman@redhat.com>
> 
> Acked-by: Daniel Borkmann <dborkman@redhat.com>

Applied and queued up for -stable, thanks.