lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 17 Oct 2011 21:28:02 +1100 From: Andrew Hendry <andrew.hendry@...il.com> To: Matthew Daley <mattjd@...il.com> Cc: netdev@...r.kernel.org, Eric Dumazet <eric.dumazet@...il.com>, stable <stable@...nel.org> Subject: Re: [PATCH 2/3] x25: Handle undersized/fragmented skbs Ran through with a lot of corrupted data, looks stable. Acked-by: Andrew Hendry <andrew.hendry@...il.com> On Sat, Oct 15, 2011 at 3:45 PM, Matthew Daley <mattjd@...il.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@...il.com> > Cc: Eric Dumazet <eric.dumazet@...il.com> > Cc: Andrew Hendry <andrew.hendry@...il.com> > Cc: stable <stable@...nel.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 > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists