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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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