[Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[P J P]

   Hello Scott, Jiri

A stack overflow issue was reported by Mr Qinghao Tang, CC'd here. It occurs 
while processing transmit(tx) descriptors in tx_consume() routine. If a 
descriptor was to have more than allowed(ROCKER_TX_FRAGS_MAX=16) packet 
fragments, the processing loop suffers an off-by-one error. Thus leading to 
OOB memory access and leakage of host memory.

Please see below a proposed patch to fix this issue. Does it look okay?

===
>From f3461d8098a0572786f5a2d7a492863090c73134 Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Tue, 22 Dec 2015 18:21:00 +0530
Subject: [PATCH] net: rocker: fix an incorrect array bounds check

While processing transmit(tx) descriptors in 'tx_consume' routine
the switch emulator suffers from an off-by-one error, if a
descriptor was to have more than allowed(ROCKER_TX_FRAGS_MAX)
fragments. Fix an incorrect bounds check to avoid it.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
  hw/net/rocker/rocker.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index c57f1a6..05102bd 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -245,7 +245,7 @@ static int tx_consume(Rocker *r, DescInfo *info)
              goto err_bad_io;
          }

-        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
+        if (++iovcnt >= ROCKER_TX_FRAGS_MAX) {
              goto err_too_many_frags;
          }
      }
-- 
2.4.3
===


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[Jiri Pirko]

Tue, Dec 22, 2015 at 02:07:01PM CET, ppandit@redhat.com wrote:
>  Hello Scott, Jiri
>
>A stack overflow issue was reported by Mr Qinghao Tang, CC'd here. It occurs
>while processing transmit(tx) descriptors in tx_consume() routine. If a
>descriptor was to have more than allowed(ROCKER_TX_FRAGS_MAX=16) packet
>fragments, the processing loop suffers an off-by-one error. Thus leading to
>OOB memory access and leakage of host memory.
>
>Please see below a proposed patch to fix this issue. Does it look okay?
>
>===
>From f3461d8098a0572786f5a2d7a492863090c73134 Mon Sep 17 00:00:00 2001
>From: Prasad J Pandit <pjp@fedoraproject.org>
>Date: Tue, 22 Dec 2015 18:21:00 +0530
>Subject: [PATCH] net: rocker: fix an incorrect array bounds check
>
>While processing transmit(tx) descriptors in 'tx_consume' routine
>the switch emulator suffers from an off-by-one error, if a
>descriptor was to have more than allowed(ROCKER_TX_FRAGS_MAX)
>fragments. Fix an incorrect bounds check to avoid it.
>
>Reported-by: Qinghao Tang <luodalongde@gmail.com>
>Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[P J P]

+-- On Tue, 22 Dec 2015, Jiri Pirko wrote --+
| >From f3461d8098a0572786f5a2d7a492863090c73134 Mon Sep 17 00:00:00 2001
| >From: Prasad J Pandit <pjp@fedoraproject.org>
| >Date: Tue, 22 Dec 2015 18:21:00 +0530
| >Subject: [PATCH] net: rocker: fix an incorrect array bounds check
| >
| >While processing transmit(tx) descriptors in 'tx_consume' routine
| >the switch emulator suffers from an off-by-one error, if a
| >descriptor was to have more than allowed(ROCKER_TX_FRAGS_MAX)
| >fragments. Fix an incorrect bounds check to avoid it.
| >
| >Reported-by: Qinghao Tang <luodalongde@gmail.com>
| >Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| 
| Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Thank you. (CC'd qemu-stable for 2.5)
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[Paolo Bonzini]

On 22/12/2015 14:07, P J P wrote:
>   Hello Scott, Jiri
> 
> A stack overflow issue was reported by Mr Qinghao Tang, CC'd here. It
> occurs while processing transmit(tx) descriptors in tx_consume()
> routine. If a descriptor was to have more than
> allowed(ROCKER_TX_FRAGS_MAX=16) packet fragments, the processing loop
> suffers an off-by-one error. Thus leading to OOB memory access and
> leakage of host memory.
> 
> Please see below a proposed patch to fix this issue. Does it look okay?
> 
> ===
> From f3461d8098a0572786f5a2d7a492863090c73134 Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Tue, 22 Dec 2015 18:21:00 +0530
> Subject: [PATCH] net: rocker: fix an incorrect array bounds check
> 
> While processing transmit(tx) descriptors in 'tx_consume' routine
> the switch emulator suffers from an off-by-one error, if a
> descriptor was to have more than allowed(ROCKER_TX_FRAGS_MAX)
> fragments. Fix an incorrect bounds check to avoid it.
> 
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/rocker/rocker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index c57f1a6..05102bd 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -245,7 +245,7 @@ static int tx_consume(Rocker *r, DescInfo *info)
>              goto err_bad_io;
>          }
> 
> -        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
> +        if (++iovcnt >= ROCKER_TX_FRAGS_MAX) {

Doesn't this forbid some valid ROCKER_TX_FRAGS_MAX-element iovecs?  The
check should be moved before the assignment to iov[iovcnt].iov_len.

Paolo

>              goto err_too_many_frags;
>          }
>      }

Re: [Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[P J P]

+-- On Tue, 22 Dec 2015, Paolo Bonzini wrote --+
| > -        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
| > +        if (++iovcnt >= ROCKER_TX_FRAGS_MAX) {
| 
| Doesn't this forbid some valid ROCKER_TX_FRAGS_MAX-element iovecs?

  forbid..?  Sorry, I did not get the question.

| The check should be moved before the assignment to iov[iovcnt].iov_len.

  You mean like below..?

===
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index c57f1a6..2e77e50 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -232,6 +232,9 @@ static int tx_consume(Rocker *r, DescInfo *info)
         frag_addr = rocker_tlv_get_le64(tlvs[ROCKER_TLV_TX_FRAG_ATTR_ADDR]);
         frag_len = rocker_tlv_get_le16(tlvs[ROCKER_TLV_TX_FRAG_ATTR_LEN]);
 
+        if (iovcnt >= ROCKER_TX_FRAGS_MAX) {
+            goto err_too_many_frags;
+        }
         iov[iovcnt].iov_len = frag_len;
         iov[iovcnt].iov_base = g_malloc(frag_len);
         if (!iov[iovcnt].iov_base) {
@@ -244,10 +247,7 @@ static int tx_consume(Rocker *r, DescInfo *info)
             err = -ROCKER_ENXIO;
             goto err_bad_io;
         }
-
-        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
-            goto err_too_many_frags;
-        }
+        iovcnt++;
     }
 
     if (iovcnt) {
===

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[Paolo Bonzini]

On 22/12/2015 18:26, P J P wrote:
> +-- On Tue, 22 Dec 2015, Paolo Bonzini wrote --+
> | > -        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
> | > +        if (++iovcnt >= ROCKER_TX_FRAGS_MAX) {
> | 
> | Doesn't this forbid some valid ROCKER_TX_FRAGS_MAX-element iovecs?
> 
>   forbid..?  Sorry, I did not get the question.
> 
> | The check should be moved before the assignment to iov[iovcnt].iov_len.
> 
>   You mean like below..?
> 
> ===
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index c57f1a6..2e77e50 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -232,6 +232,9 @@ static int tx_consume(Rocker *r, DescInfo *info)
>          frag_addr = rocker_tlv_get_le64(tlvs[ROCKER_TLV_TX_FRAG_ATTR_ADDR]);
>          frag_len = rocker_tlv_get_le16(tlvs[ROCKER_TLV_TX_FRAG_ATTR_LEN]);
>  
> +        if (iovcnt >= ROCKER_TX_FRAGS_MAX) {
> +            goto err_too_many_frags;
> +        }
>          iov[iovcnt].iov_len = frag_len;
>          iov[iovcnt].iov_base = g_malloc(frag_len);
>          if (!iov[iovcnt].iov_base) {
> @@ -244,10 +247,7 @@ static int tx_consume(Rocker *r, DescInfo *info)
>              err = -ROCKER_ENXIO;
>              goto err_bad_io;
>          }
> -
> -        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
> -            goto err_too_many_frags;
> -        }
> +        iovcnt++;
>      }
>  
>      if (iovcnt) {
> ===
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
> 
> 

Yes, can you submit it as v2?

Paolo

Re: [Qemu-devel] [PATCH] net: rocker: fix an incorrect array bounds check

[P J P]

+-- On Tue, 22 Dec 2015, Paolo Bonzini wrote --+
| > ===
| > diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
| > index c57f1a6..2e77e50 100644
| > --- a/hw/net/rocker/rocker.c
| > +++ b/hw/net/rocker/rocker.c
| > @@ -232,6 +232,9 @@ static int tx_consume(Rocker *r, DescInfo *info)
| >          frag_addr = rocker_tlv_get_le64(tlvs[ROCKER_TLV_TX_FRAG_ATTR_ADDR]);
| >          frag_len = rocker_tlv_get_le16(tlvs[ROCKER_TLV_TX_FRAG_ATTR_LEN]);
| >  
| > +        if (iovcnt >= ROCKER_TX_FRAGS_MAX) {
| > +            goto err_too_many_frags;
| > +        }
| >          iov[iovcnt].iov_len = frag_len;
| >          iov[iovcnt].iov_base = g_malloc(frag_len);
| >          if (!iov[iovcnt].iov_base) {
| > @@ -244,10 +247,7 @@ static int tx_consume(Rocker *r, DescInfo *info)
| >              err = -ROCKER_ENXIO;
| >              goto err_bad_io;
| >          }
| > -
| > -        if (++iovcnt > ROCKER_TX_FRAGS_MAX) {
| > -            goto err_too_many_frags;
| > -        }
| > +        iovcnt++;
| >      }
| >  
| >      if (iovcnt) {
| > ===
| 
| Yes, can you submit it as v2?

Sure, okay. But I don't get the difference.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F