Skip to contentSkip to conversation

net: ax25: fix information leak to userland

PATCH Bernard Pidoux 1 participant 1 message Nov 1, 2010 Networking

lore
Share & tools
lore

Bernard Pidoux

author
#0 lore raw

Hi,

I applied the AX25 patch and did not observe any problem with AX25 nor NetROM or ROSE traffic.

I observed that a similar structure initialization existed in rose_getname() but not in netrom nr_getname().

A similar patch for af_netrom could be :

net/netrom/af_netrom.c +1 -1
@@ -840,6 +840,7 @@ static int nr_getname(struct socket *soc
 	struct sock *sk = sock->sk;
 	struct nr_sock *nr = nr_sk(sk);
 
+	memset(sax, 0, sizeof(*sax));
 	lock_sock(sk);
 	if (peer != 0) {
 		if (sk->sk_state != TCP_ESTABLISHED) {

@@ -854,7 +855,6 @@ static int nr_getname(struct socket *soc
 		*uaddr_len = sizeof(struct full_sockaddr_ax25);
 	} else {
 		sax->fsa_ax25.sax25_family = AF_NETROM;
-		sax->fsa_ax25.sax25_ndigis = 0;
 		sax->fsa_ax25.sax25_call   = nr->source_addr;
 		*uaddr_len = sizeof(struct sockaddr_ax25);
 	}
Signed-off-by Bernard Pidoux<[email protected]>

Le dimanche 31 octobre 2010 à 20:10 +0300, Vasiliy Kulikov a écrit :

Quoted 17 lines

Sometimes ax25_getname() doesn't initialize all members of fsa_digipeater field of fsa struct. This structure is then copied to userland. It leads to leaking of contents of kernel stack memory. We have to initialize them to zero.

Signed-off-by Vasiliy Kulikov<[email protected]>

---

  net/ax25/af_ax25.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 26eaebf..a324d83 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1392,6 +1392,7 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
ax25_cb *ax25;
int err = 0;

Quoted 4 lines

+ memset(&fsa->fsa_digipeater, 0, sizeof(fsa->fsa_digipeater));
lock_sock(sk);
ax25 = ax25_sk(sk);

If you really want to fix this for good, please do it completely ?

sa_family_t is a short

ax25_address is 7 bytes.

Therefore, there is a hole before sax25_ndigis.

struct sockaddr_ax25 {
        sa_family_t     sax25_family;
        ax25_address    sax25_call;
<hole>
        int             sax25_ndigis;
        /* Digipeater ax25_address sets follow */
};
struct full_sockaddr_ax25 {
        struct sockaddr_ax25 fsa_ax25;
        ax25_address    fsa_digipeater[AX25_MAX_DIGIS];
};

So a correct patch is the following one. Note AX25 is probably used by nobody at all, so a full memset() is not performance critical in this path.

net/ax25/af_ax25.c +1 -1
index 26eaebf..6da5dae 100644
@@ -1392,6 +1392,7 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
 	ax25_cb *ax25;
 	int err = 0;
 
+	memset(fsa, 0, sizeof(*fsa));
 	lock_sock(sk);
 	ax25 = ax25_sk(sk);
 

@@ -1403,7 +1404,6 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
 
 		fsa->fsa_ax25.sax25_family = AF_AX25;
 		fsa->fsa_ax25.sax25_call   = ax25->dest_addr;
-		fsa->fsa_ax25.sax25_ndigis = 0;
 
 		if (ax25->digipeat != NULL) {
 			ndigi = ax25->digipeat->ndigi;

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