* x25: Fix multiple buffer overruns/overreads
@ 2011-10-15 4:45 Matthew Daley
2011-10-15 4:45 ` [PATCH 1/3] x25: Validate incoming call user data lengths Matthew Daley
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Matthew Daley @ 2011-10-15 4:45 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, Andrew Hendry
This patchset fixes several buffer overruns/overreads in the X.25
packet layer. The first patch fixes a particularly nasty remote-triggerable
buffer overflow, while the rest fix skb overreads on undersized/fragmented
skbs.
Matthew Daley (3):
x25: Validate incoming call user data lengths
x25: Handle undersized/fragmented skbs
x25: Prevent skb overreads when checking call user data
net/x25/af_x25.c | 40 ++++++++++++++++++++++++++++++++--------
net/x25/x25_dev.c | 6 ++++++
net/x25/x25_facilities.c | 10 ++++++----
net/x25/x25_in.c | 43 ++++++++++++++++++++++++++++++++++++++-----
net/x25/x25_link.c | 3 +++
net/x25/x25_subr.c | 14 +++++++++++++-
6 files changed, 98 insertions(+), 18 deletions(-)
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/3] x25: Validate incoming call user data lengths 2011-10-15 4:45 x25: Fix multiple buffer overruns/overreads Matthew Daley @ 2011-10-15 4:45 ` Matthew Daley 2011-10-15 4:45 ` [PATCH 2/3] x25: Handle undersized/fragmented skbs Matthew Daley ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Matthew Daley @ 2011-10-15 4:45 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, Andrew Hendry, Matthew Daley, stable X.25 call user data is being copied in its entirety from incoming messages without consideration to the size of the destination buffers, leading to possible buffer overflows. Validate incoming call user data lengths before these copies are performed. It appears this issue was noticed some time ago, however nothing seemed to come of it: see http://www.spinics.net/lists/linux-x25/msg00043.html and commit 8db09f26f912f7c90c764806e804b558da520d4f. Signed-off-by: Matthew Daley <mattjd@gmail.com> Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Tested-by: Andrew Hendry <andrew.hendry@gmail.com> Cc: stable <stable@kernel.org> --- net/x25/af_x25.c | 6 ++++++ net/x25/x25_in.c | 3 +++ 2 files changed, 9 insertions(+), 0 deletions(-) diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index d306154..a4bd172 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -959,6 +959,12 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb, skb_pull(skb,len); /* + * Ensure that the amount of call user data is valid. + */ + if (skb->len > X25_MAX_CUD_LEN) + goto out_clear_request; + + /* * Find a listener for the particular address/cud pair. */ sk = x25_find_listener(&source_addr,skb); diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c index 0b073b5..63488fd 100644 --- a/net/x25/x25_in.c +++ b/net/x25/x25_in.c @@ -127,6 +127,9 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp * Copy any Call User Data. */ if (skb->len > 0) { + if (skb->len > X25_MAX_CUD_LEN) + goto out_clear; + skb_copy_from_linear_data(skb, x25->calluserdata.cuddata, skb->len); -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] x25: Handle undersized/fragmented skbs 2011-10-15 4:45 x25: Fix multiple buffer overruns/overreads Matthew Daley 2011-10-15 4:45 ` [PATCH 1/3] x25: Validate incoming call user data lengths Matthew Daley @ 2011-10-15 4:45 ` Matthew Daley 2011-10-17 10:28 ` Andrew Hendry 2011-10-15 4:45 ` [PATCH 3/3] x25: Prevent skb overreads when checking call user data Matthew Daley 2011-10-17 23:32 ` x25: Fix multiple buffer overruns/overreads David Miller 3 siblings, 1 reply; 7+ messages in thread From: Matthew Daley @ 2011-10-15 4:45 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, Andrew Hendry, Matthew Daley, stable There are multiple locations in the X.25 packet layer where a skb is assumed to be of at least a certain size and that all its data is currently available at skb->data. These assumptions are not checked, hence buffer overreads may occur. Use pskb_may_pull to check these minimal size assumptions and ensure that data is available at skb->data when necessary, as well as use skb_copy_bits where needed. Signed-off-by: Matthew Daley <mattjd@gmail.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Andrew Hendry <andrew.hendry@gmail.com> Cc: stable <stable@kernel.org> --- net/x25/af_x25.c | 31 ++++++++++++++++++++++++------- net/x25/x25_dev.c | 6 ++++++ net/x25/x25_facilities.c | 10 ++++++---- net/x25/x25_in.c | 40 +++++++++++++++++++++++++++++++++++----- net/x25/x25_link.c | 3 +++ net/x25/x25_subr.c | 14 +++++++++++++- 6 files changed, 87 insertions(+), 17 deletions(-) diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index a4bd172..aa567b0 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -91,7 +91,7 @@ int x25_parse_address_block(struct sk_buff *skb, int needed; int rc; - if (skb->len < 1) { + if (!pskb_may_pull(skb, 1)) { /* packet has no address block */ rc = 0; goto empty; @@ -100,7 +100,7 @@ int x25_parse_address_block(struct sk_buff *skb, len = *skb->data; needed = 1 + (len >> 4) + (len & 0x0f); - if (skb->len < needed) { + if (!pskb_may_pull(skb, needed)) { /* packet is too short to hold the addresses it claims to hold */ rc = -1; @@ -951,10 +951,10 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb, * * Facilities length is mandatory in call request packets */ - if (skb->len < 1) + if (!pskb_may_pull(skb, 1)) goto out_clear_request; len = skb->data[0] + 1; - if (skb->len < len) + if (!pskb_may_pull(skb, len)) goto out_clear_request; skb_pull(skb,len); @@ -965,6 +965,13 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb, goto out_clear_request; /* + * Get all the call user data so it can be used in + * x25_find_listener and skb_copy_from_linear_data up ahead. + */ + if (!pskb_may_pull(skb, skb->len)) + goto out_clear_request; + + /* * Find a listener for the particular address/cud pair. */ sk = x25_find_listener(&source_addr,skb); @@ -1172,6 +1179,9 @@ static int x25_sendmsg(struct kiocb *iocb, struct socket *sock, * byte of the user data is the logical value of the Q Bit. */ if (test_bit(X25_Q_BIT_FLAG, &x25->flags)) { + if (!pskb_may_pull(skb, 1)) + goto out_kfree_skb; + qbit = skb->data[0]; skb_pull(skb, 1); } @@ -1250,7 +1260,9 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock, struct x25_sock *x25 = x25_sk(sk); struct sockaddr_x25 *sx25 = (struct sockaddr_x25 *)msg->msg_name; size_t copied; - int qbit; + int qbit, header_len = x25->neighbour->extended ? + X25_EXT_MIN_LEN : X25_STD_MIN_LEN; + struct sk_buff *skb; unsigned char *asmptr; int rc = -ENOTCONN; @@ -1271,6 +1283,9 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock, skb = skb_dequeue(&x25->interrupt_in_queue); + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) + goto out_free_dgram; + skb_pull(skb, X25_STD_MIN_LEN); /* @@ -1291,10 +1306,12 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock, if (!skb) goto out; + if (!pskb_may_pull(skb, header_len)) + goto out_free_dgram; + qbit = (skb->data[0] & X25_Q_BIT) == X25_Q_BIT; - skb_pull(skb, x25->neighbour->extended ? - X25_EXT_MIN_LEN : X25_STD_MIN_LEN); + skb_pull(skb, header_len); if (test_bit(X25_Q_BIT_FLAG, &x25->flags)) { asmptr = skb_push(skb, 1); diff --git a/net/x25/x25_dev.c b/net/x25/x25_dev.c index e547ca1..fa2b418 100644 --- a/net/x25/x25_dev.c +++ b/net/x25/x25_dev.c @@ -32,6 +32,9 @@ static int x25_receive_data(struct sk_buff *skb, struct x25_neigh *nb) unsigned short frametype; unsigned int lci; + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) + return 0; + frametype = skb->data[2]; lci = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF); @@ -115,6 +118,9 @@ int x25_lapb_receive_frame(struct sk_buff *skb, struct net_device *dev, goto drop; } + if (!pskb_may_pull(skb, 1)) + return 0; + switch (skb->data[0]) { case X25_IFACE_DATA: diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c index f77e4e7..36384a1 100644 --- a/net/x25/x25_facilities.c +++ b/net/x25/x25_facilities.c @@ -44,7 +44,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, struct x25_dte_facilities *dte_facs, unsigned long *vc_fac_mask) { - unsigned char *p = skb->data; + unsigned char *p; unsigned int len; *vc_fac_mask = 0; @@ -60,14 +60,16 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, memset(dte_facs->called_ae, '\0', sizeof(dte_facs->called_ae)); memset(dte_facs->calling_ae, '\0', sizeof(dte_facs->calling_ae)); - if (skb->len < 1) + if (!pskb_may_pull(skb, 1)) return 0; - len = *p++; + len = skb->data[0]; - if (len >= skb->len) + if (!pskb_may_pull(skb, 1 + len)) return -1; + p = skb->data + 1; + while (len > 0) { switch (*p & X25_FAC_CLASS_MASK) { case X25_FAC_CLASS_A: diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c index 63488fd..a49cd4e 100644 --- a/net/x25/x25_in.c +++ b/net/x25/x25_in.c @@ -107,6 +107,8 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp /* * Parse the data in the frame. */ + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) + goto out_clear; skb_pull(skb, X25_STD_MIN_LEN); len = x25_parse_address_block(skb, &source_addr, @@ -130,9 +132,8 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp if (skb->len > X25_MAX_CUD_LEN) goto out_clear; - skb_copy_from_linear_data(skb, - x25->calluserdata.cuddata, - skb->len); + skb_copy_bits(skb, 0, x25->calluserdata.cuddata, + skb->len); x25->calluserdata.cudlength = skb->len; } if (!sock_flag(sk, SOCK_DEAD)) @@ -140,6 +141,9 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp break; } case X25_CLEAR_REQUEST: + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) + goto out_clear; + x25_write_internal(sk, X25_CLEAR_CONFIRMATION); x25_disconnect(sk, ECONNREFUSED, skb->data[3], skb->data[4]); break; @@ -167,6 +171,9 @@ static int x25_state2_machine(struct sock *sk, struct sk_buff *skb, int frametyp switch (frametype) { case X25_CLEAR_REQUEST: + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) + goto out_clear; + x25_write_internal(sk, X25_CLEAR_CONFIRMATION); x25_disconnect(sk, 0, skb->data[3], skb->data[4]); break; @@ -180,6 +187,11 @@ static int x25_state2_machine(struct sock *sk, struct sk_buff *skb, int frametyp } return 0; + +out_clear: + x25_write_internal(sk, X25_CLEAR_REQUEST); + x25_start_t23timer(sk); + return 0; } /* @@ -209,6 +221,9 @@ static int x25_state3_machine(struct sock *sk, struct sk_buff *skb, int frametyp break; case X25_CLEAR_REQUEST: + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) + goto out_clear; + x25_write_internal(sk, X25_CLEAR_CONFIRMATION); x25_disconnect(sk, 0, skb->data[3], skb->data[4]); break; @@ -307,6 +322,12 @@ static int x25_state3_machine(struct sock *sk, struct sk_buff *skb, int frametyp } return queued; + +out_clear: + x25_write_internal(sk, X25_CLEAR_REQUEST); + x25->state = X25_STATE_2; + x25_start_t23timer(sk); + return 0; } /* @@ -316,13 +337,13 @@ static int x25_state3_machine(struct sock *sk, struct sk_buff *skb, int frametyp */ static int x25_state4_machine(struct sock *sk, struct sk_buff *skb, int frametype) { + struct x25_sock *x25 = x25_sk(sk); + switch (frametype) { case X25_RESET_REQUEST: x25_write_internal(sk, X25_RESET_CONFIRMATION); case X25_RESET_CONFIRMATION: { - struct x25_sock *x25 = x25_sk(sk); - x25_stop_timer(sk); x25->condition = 0x00; x25->va = 0; @@ -334,6 +355,9 @@ static int x25_state4_machine(struct sock *sk, struct sk_buff *skb, int frametyp break; } case X25_CLEAR_REQUEST: + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) + goto out_clear; + x25_write_internal(sk, X25_CLEAR_CONFIRMATION); x25_disconnect(sk, 0, skb->data[3], skb->data[4]); break; @@ -343,6 +367,12 @@ static int x25_state4_machine(struct sock *sk, struct sk_buff *skb, int frametyp } return 0; + +out_clear: + x25_write_internal(sk, X25_CLEAR_REQUEST); + x25->state = X25_STATE_2; + x25_start_t23timer(sk); + return 0; } /* Higher level upcall for a LAPB frame */ diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c index 037958f..4acacf3 100644 --- a/net/x25/x25_link.c +++ b/net/x25/x25_link.c @@ -90,6 +90,9 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh *nb, break; case X25_DIAGNOSTIC: + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 4)) + break; + printk(KERN_WARNING "x25: diagnostic #%d - %02X %02X %02X\n", skb->data[3], skb->data[4], skb->data[5], skb->data[6]); diff --git a/net/x25/x25_subr.c b/net/x25/x25_subr.c index 24a342e..5170d52 100644 --- a/net/x25/x25_subr.c +++ b/net/x25/x25_subr.c @@ -269,7 +269,11 @@ int x25_decode(struct sock *sk, struct sk_buff *skb, int *ns, int *nr, int *q, int *d, int *m) { struct x25_sock *x25 = x25_sk(sk); - unsigned char *frame = skb->data; + unsigned char *frame; + + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) + return X25_ILLEGAL; + frame = skb->data; *ns = *nr = *q = *d = *m = 0; @@ -294,6 +298,10 @@ int x25_decode(struct sock *sk, struct sk_buff *skb, int *ns, int *nr, int *q, if (frame[2] == X25_RR || frame[2] == X25_RNR || frame[2] == X25_REJ) { + if (!pskb_may_pull(skb, X25_EXT_MIN_LEN)) + return X25_ILLEGAL; + frame = skb->data; + *nr = (frame[3] >> 1) & 0x7F; return frame[2]; } @@ -308,6 +316,10 @@ int x25_decode(struct sock *sk, struct sk_buff *skb, int *ns, int *nr, int *q, if (x25->neighbour->extended) { if ((frame[2] & 0x01) == X25_DATA) { + if (!pskb_may_pull(skb, X25_EXT_MIN_LEN)) + return X25_ILLEGAL; + frame = skb->data; + *q = (frame[0] & X25_Q_BIT) == X25_Q_BIT; *d = (frame[0] & X25_D_BIT) == X25_D_BIT; *m = (frame[3] & X25_EXT_M_BIT) == X25_EXT_M_BIT; -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] x25: Handle undersized/fragmented skbs 2011-10-15 4:45 ` [PATCH 2/3] x25: Handle undersized/fragmented skbs Matthew Daley @ 2011-10-17 10:28 ` Andrew Hendry 0 siblings, 0 replies; 7+ messages in thread From: Andrew Hendry @ 2011-10-17 10:28 UTC (permalink / raw) To: Matthew Daley; +Cc: netdev, Eric Dumazet, stable Ran through with a lot of corrupted data, looks stable. Acked-by: Andrew Hendry <andrew.hendry@gmail.com> On Sat, Oct 15, 2011 at 3:45 PM, Matthew Daley <mattjd@gmail.com> wrote: > There are multiple locations in the X.25 packet layer where a skb is > assumed to be of at least a certain size and that all its data is > currently available at skb->data. These assumptions are not checked, > hence buffer overreads may occur. Use pskb_may_pull to check these > minimal size assumptions and ensure that data is available at skb->data > when necessary, as well as use skb_copy_bits where needed. > > Signed-off-by: Matthew Daley <mattjd@gmail.com> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Andrew Hendry <andrew.hendry@gmail.com> > Cc: stable <stable@kernel.org> > --- > net/x25/af_x25.c | 31 ++++++++++++++++++++++++------- > net/x25/x25_dev.c | 6 ++++++ > net/x25/x25_facilities.c | 10 ++++++---- > net/x25/x25_in.c | 40 +++++++++++++++++++++++++++++++++++----- > net/x25/x25_link.c | 3 +++ > net/x25/x25_subr.c | 14 +++++++++++++- > 6 files changed, 87 insertions(+), 17 deletions(-) > > diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c > index a4bd172..aa567b0 100644 > --- a/net/x25/af_x25.c > +++ b/net/x25/af_x25.c > @@ -91,7 +91,7 @@ int x25_parse_address_block(struct sk_buff *skb, > int needed; > int rc; > > - if (skb->len < 1) { > + if (!pskb_may_pull(skb, 1)) { > /* packet has no address block */ > rc = 0; > goto empty; > @@ -100,7 +100,7 @@ int x25_parse_address_block(struct sk_buff *skb, > len = *skb->data; > needed = 1 + (len >> 4) + (len & 0x0f); > > - if (skb->len < needed) { > + if (!pskb_may_pull(skb, needed)) { > /* packet is too short to hold the addresses it claims > to hold */ > rc = -1; > @@ -951,10 +951,10 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb, > * > * Facilities length is mandatory in call request packets > */ > - if (skb->len < 1) > + if (!pskb_may_pull(skb, 1)) > goto out_clear_request; > len = skb->data[0] + 1; > - if (skb->len < len) > + if (!pskb_may_pull(skb, len)) > goto out_clear_request; > skb_pull(skb,len); > > @@ -965,6 +965,13 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb, > goto out_clear_request; > > /* > + * Get all the call user data so it can be used in > + * x25_find_listener and skb_copy_from_linear_data up ahead. > + */ > + if (!pskb_may_pull(skb, skb->len)) > + goto out_clear_request; > + > + /* > * Find a listener for the particular address/cud pair. > */ > sk = x25_find_listener(&source_addr,skb); > @@ -1172,6 +1179,9 @@ static int x25_sendmsg(struct kiocb *iocb, struct socket *sock, > * byte of the user data is the logical value of the Q Bit. > */ > if (test_bit(X25_Q_BIT_FLAG, &x25->flags)) { > + if (!pskb_may_pull(skb, 1)) > + goto out_kfree_skb; > + > qbit = skb->data[0]; > skb_pull(skb, 1); > } > @@ -1250,7 +1260,9 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock, > struct x25_sock *x25 = x25_sk(sk); > struct sockaddr_x25 *sx25 = (struct sockaddr_x25 *)msg->msg_name; > size_t copied; > - int qbit; > + int qbit, header_len = x25->neighbour->extended ? > + X25_EXT_MIN_LEN : X25_STD_MIN_LEN; > + > struct sk_buff *skb; > unsigned char *asmptr; > int rc = -ENOTCONN; > @@ -1271,6 +1283,9 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock, > > skb = skb_dequeue(&x25->interrupt_in_queue); > > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) > + goto out_free_dgram; > + > skb_pull(skb, X25_STD_MIN_LEN); > > /* > @@ -1291,10 +1306,12 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock, > if (!skb) > goto out; > > + if (!pskb_may_pull(skb, header_len)) > + goto out_free_dgram; > + > qbit = (skb->data[0] & X25_Q_BIT) == X25_Q_BIT; > > - skb_pull(skb, x25->neighbour->extended ? > - X25_EXT_MIN_LEN : X25_STD_MIN_LEN); > + skb_pull(skb, header_len); > > if (test_bit(X25_Q_BIT_FLAG, &x25->flags)) { > asmptr = skb_push(skb, 1); > diff --git a/net/x25/x25_dev.c b/net/x25/x25_dev.c > index e547ca1..fa2b418 100644 > --- a/net/x25/x25_dev.c > +++ b/net/x25/x25_dev.c > @@ -32,6 +32,9 @@ static int x25_receive_data(struct sk_buff *skb, struct x25_neigh *nb) > unsigned short frametype; > unsigned int lci; > > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) > + return 0; > + > frametype = skb->data[2]; > lci = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF); > > @@ -115,6 +118,9 @@ int x25_lapb_receive_frame(struct sk_buff *skb, struct net_device *dev, > goto drop; > } > > + if (!pskb_may_pull(skb, 1)) > + return 0; > + > switch (skb->data[0]) { > > case X25_IFACE_DATA: > diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c > index f77e4e7..36384a1 100644 > --- a/net/x25/x25_facilities.c > +++ b/net/x25/x25_facilities.c > @@ -44,7 +44,7 @@ > int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > struct x25_dte_facilities *dte_facs, unsigned long *vc_fac_mask) > { > - unsigned char *p = skb->data; > + unsigned char *p; > unsigned int len; > > *vc_fac_mask = 0; > @@ -60,14 +60,16 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > memset(dte_facs->called_ae, '\0', sizeof(dte_facs->called_ae)); > memset(dte_facs->calling_ae, '\0', sizeof(dte_facs->calling_ae)); > > - if (skb->len < 1) > + if (!pskb_may_pull(skb, 1)) > return 0; > > - len = *p++; > + len = skb->data[0]; > > - if (len >= skb->len) > + if (!pskb_may_pull(skb, 1 + len)) > return -1; > > + p = skb->data + 1; > + > while (len > 0) { > switch (*p & X25_FAC_CLASS_MASK) { > case X25_FAC_CLASS_A: > diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c > index 63488fd..a49cd4e 100644 > --- a/net/x25/x25_in.c > +++ b/net/x25/x25_in.c > @@ -107,6 +107,8 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > /* > * Parse the data in the frame. > */ > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) > + goto out_clear; > skb_pull(skb, X25_STD_MIN_LEN); > > len = x25_parse_address_block(skb, &source_addr, > @@ -130,9 +132,8 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > if (skb->len > X25_MAX_CUD_LEN) > goto out_clear; > > - skb_copy_from_linear_data(skb, > - x25->calluserdata.cuddata, > - skb->len); > + skb_copy_bits(skb, 0, x25->calluserdata.cuddata, > + skb->len); > x25->calluserdata.cudlength = skb->len; > } > if (!sock_flag(sk, SOCK_DEAD)) > @@ -140,6 +141,9 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > break; > } > case X25_CLEAR_REQUEST: > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) > + goto out_clear; > + > x25_write_internal(sk, X25_CLEAR_CONFIRMATION); > x25_disconnect(sk, ECONNREFUSED, skb->data[3], skb->data[4]); > break; > @@ -167,6 +171,9 @@ static int x25_state2_machine(struct sock *sk, struct sk_buff *skb, int frametyp > switch (frametype) { > > case X25_CLEAR_REQUEST: > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) > + goto out_clear; > + > x25_write_internal(sk, X25_CLEAR_CONFIRMATION); > x25_disconnect(sk, 0, skb->data[3], skb->data[4]); > break; > @@ -180,6 +187,11 @@ static int x25_state2_machine(struct sock *sk, struct sk_buff *skb, int frametyp > } > > return 0; > + > +out_clear: > + x25_write_internal(sk, X25_CLEAR_REQUEST); > + x25_start_t23timer(sk); > + return 0; > } > > /* > @@ -209,6 +221,9 @@ static int x25_state3_machine(struct sock *sk, struct sk_buff *skb, int frametyp > break; > > case X25_CLEAR_REQUEST: > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) > + goto out_clear; > + > x25_write_internal(sk, X25_CLEAR_CONFIRMATION); > x25_disconnect(sk, 0, skb->data[3], skb->data[4]); > break; > @@ -307,6 +322,12 @@ static int x25_state3_machine(struct sock *sk, struct sk_buff *skb, int frametyp > } > > return queued; > + > +out_clear: > + x25_write_internal(sk, X25_CLEAR_REQUEST); > + x25->state = X25_STATE_2; > + x25_start_t23timer(sk); > + return 0; > } > > /* > @@ -316,13 +337,13 @@ static int x25_state3_machine(struct sock *sk, struct sk_buff *skb, int frametyp > */ > static int x25_state4_machine(struct sock *sk, struct sk_buff *skb, int frametype) > { > + struct x25_sock *x25 = x25_sk(sk); > + > switch (frametype) { > > case X25_RESET_REQUEST: > x25_write_internal(sk, X25_RESET_CONFIRMATION); > case X25_RESET_CONFIRMATION: { > - struct x25_sock *x25 = x25_sk(sk); > - > x25_stop_timer(sk); > x25->condition = 0x00; > x25->va = 0; > @@ -334,6 +355,9 @@ static int x25_state4_machine(struct sock *sk, struct sk_buff *skb, int frametyp > break; > } > case X25_CLEAR_REQUEST: > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 2)) > + goto out_clear; > + > x25_write_internal(sk, X25_CLEAR_CONFIRMATION); > x25_disconnect(sk, 0, skb->data[3], skb->data[4]); > break; > @@ -343,6 +367,12 @@ static int x25_state4_machine(struct sock *sk, struct sk_buff *skb, int frametyp > } > > return 0; > + > +out_clear: > + x25_write_internal(sk, X25_CLEAR_REQUEST); > + x25->state = X25_STATE_2; > + x25_start_t23timer(sk); > + return 0; > } > > /* Higher level upcall for a LAPB frame */ > diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c > index 037958f..4acacf3 100644 > --- a/net/x25/x25_link.c > +++ b/net/x25/x25_link.c > @@ -90,6 +90,9 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh *nb, > break; > > case X25_DIAGNOSTIC: > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN + 4)) > + break; > + > printk(KERN_WARNING "x25: diagnostic #%d - %02X %02X %02X\n", > skb->data[3], skb->data[4], > skb->data[5], skb->data[6]); > diff --git a/net/x25/x25_subr.c b/net/x25/x25_subr.c > index 24a342e..5170d52 100644 > --- a/net/x25/x25_subr.c > +++ b/net/x25/x25_subr.c > @@ -269,7 +269,11 @@ int x25_decode(struct sock *sk, struct sk_buff *skb, int *ns, int *nr, int *q, > int *d, int *m) > { > struct x25_sock *x25 = x25_sk(sk); > - unsigned char *frame = skb->data; > + unsigned char *frame; > + > + if (!pskb_may_pull(skb, X25_STD_MIN_LEN)) > + return X25_ILLEGAL; > + frame = skb->data; > > *ns = *nr = *q = *d = *m = 0; > > @@ -294,6 +298,10 @@ int x25_decode(struct sock *sk, struct sk_buff *skb, int *ns, int *nr, int *q, > if (frame[2] == X25_RR || > frame[2] == X25_RNR || > frame[2] == X25_REJ) { > + if (!pskb_may_pull(skb, X25_EXT_MIN_LEN)) > + return X25_ILLEGAL; > + frame = skb->data; > + > *nr = (frame[3] >> 1) & 0x7F; > return frame[2]; > } > @@ -308,6 +316,10 @@ int x25_decode(struct sock *sk, struct sk_buff *skb, int *ns, int *nr, int *q, > > if (x25->neighbour->extended) { > if ((frame[2] & 0x01) == X25_DATA) { > + if (!pskb_may_pull(skb, X25_EXT_MIN_LEN)) > + return X25_ILLEGAL; > + frame = skb->data; > + > *q = (frame[0] & X25_Q_BIT) == X25_Q_BIT; > *d = (frame[0] & X25_D_BIT) == X25_D_BIT; > *m = (frame[3] & X25_EXT_M_BIT) == X25_EXT_M_BIT; > -- > 1.7.2.5 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] x25: Prevent skb overreads when checking call user data 2011-10-15 4:45 x25: Fix multiple buffer overruns/overreads Matthew Daley 2011-10-15 4:45 ` [PATCH 1/3] x25: Validate incoming call user data lengths Matthew Daley 2011-10-15 4:45 ` [PATCH 2/3] x25: Handle undersized/fragmented skbs Matthew Daley @ 2011-10-15 4:45 ` Matthew Daley 2011-10-17 10:28 ` Andrew Hendry 2011-10-17 23:32 ` x25: Fix multiple buffer overruns/overreads David Miller 3 siblings, 1 reply; 7+ messages in thread From: Matthew Daley @ 2011-10-15 4:45 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, Andrew Hendry, Matthew Daley, stable x25_find_listener does not check that the amount of call user data given in the skb is big enough in per-socket comparisons, hence buffer overreads may occur. Fix this by adding a check. Signed-off-by: Matthew Daley <mattjd@gmail.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Andrew Hendry <andrew.hendry@gmail.com> Cc: stable <stable@kernel.org> --- net/x25/af_x25.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index aa567b0..5f03e4e 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -295,7 +295,8 @@ static struct sock *x25_find_listener(struct x25_address *addr, * Found a listening socket, now check the incoming * call user data vs this sockets call user data */ - if(skb->len > 0 && x25_sk(s)->cudmatchlength > 0) { + if (x25_sk(s)->cudmatchlength > 0 && + skb->len >= x25_sk(s)->cudmatchlength) { if((memcmp(x25_sk(s)->calluserdata.cuddata, skb->data, x25_sk(s)->cudmatchlength)) == 0) { -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] x25: Prevent skb overreads when checking call user data 2011-10-15 4:45 ` [PATCH 3/3] x25: Prevent skb overreads when checking call user data Matthew Daley @ 2011-10-17 10:28 ` Andrew Hendry 0 siblings, 0 replies; 7+ messages in thread From: Andrew Hendry @ 2011-10-17 10:28 UTC (permalink / raw) To: Matthew Daley; +Cc: netdev, Eric Dumazet, stable Acked-by: Andrew Hendry <andrew.hendry@gmail.com> On Sat, Oct 15, 2011 at 3:45 PM, Matthew Daley <mattjd@gmail.com> wrote: > x25_find_listener does not check that the amount of call user data given > in the skb is big enough in per-socket comparisons, hence buffer > overreads may occur. Fix this by adding a check. > > Signed-off-by: Matthew Daley <mattjd@gmail.com> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Andrew Hendry <andrew.hendry@gmail.com> > Cc: stable <stable@kernel.org> > --- > net/x25/af_x25.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c > index aa567b0..5f03e4e 100644 > --- a/net/x25/af_x25.c > +++ b/net/x25/af_x25.c > @@ -295,7 +295,8 @@ static struct sock *x25_find_listener(struct x25_address *addr, > * Found a listening socket, now check the incoming > * call user data vs this sockets call user data > */ > - if(skb->len > 0 && x25_sk(s)->cudmatchlength > 0) { > + if (x25_sk(s)->cudmatchlength > 0 && > + skb->len >= x25_sk(s)->cudmatchlength) { > if((memcmp(x25_sk(s)->calluserdata.cuddata, > skb->data, > x25_sk(s)->cudmatchlength)) == 0) { > -- > 1.7.2.5 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: x25: Fix multiple buffer overruns/overreads 2011-10-15 4:45 x25: Fix multiple buffer overruns/overreads Matthew Daley ` (2 preceding siblings ...) 2011-10-15 4:45 ` [PATCH 3/3] x25: Prevent skb overreads when checking call user data Matthew Daley @ 2011-10-17 23:32 ` David Miller 3 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2011-10-17 23:32 UTC (permalink / raw) To: mattjd; +Cc: netdev, eric.dumazet, andrew.hendry From: Matthew Daley <mattjd@gmail.com> Date: Sat, 15 Oct 2011 00:45:02 -0400 > This patchset fixes several buffer overruns/overreads in the X.25 > packet layer. The first patch fixes a particularly nasty remote-triggerable > buffer overflow, while the rest fix skb overreads on undersized/fragmented > skbs. All applied, thanks Matthew! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-10-17 23:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-15 4:45 x25: Fix multiple buffer overruns/overreads Matthew Daley 2011-10-15 4:45 ` [PATCH 1/3] x25: Validate incoming call user data lengths Matthew Daley 2011-10-15 4:45 ` [PATCH 2/3] x25: Handle undersized/fragmented skbs Matthew Daley 2011-10-17 10:28 ` Andrew Hendry 2011-10-15 4:45 ` [PATCH 3/3] x25: Prevent skb overreads when checking call user data Matthew Daley 2011-10-17 10:28 ` Andrew Hendry 2011-10-17 23:32 ` x25: Fix multiple buffer overruns/overreads David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).