| 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
| ||
|
Message-ID: <CADo0ohiqcN=xJDrJYg8P86v4karbaLFWL27ARtEHmEAqHnnewQ@mail.gmail.com>
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