[Qemu-devel] [PATCH] net: cadence_gem: check packet size in gem_recieve

[Qemu-devel] [PATCH] net: cadence_gem: check packet size in gem_recieve

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

While receiving packets in 'gem_receive' routine, if Frame Check
Sequence(FCS) is enabled, it copies the packet into a local
buffer without checking its size. Add check to validate packet
length against the buffer size to avoid buffer overflow.

Reported-by: Ling Liu <liuling-it@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/cadence_gem.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3639fc1..c3a273b 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -677,10 +677,14 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     } else {
         unsigned crc_val;
 
+        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
+            size = sizeof(rxbuf) - sizeof(crc_val);
+        }
+        bytes_to_copy = size;
+
         /* The application wants the FCS field, which QEMU does not provide.
          * We must try and calculate one.
          */
-
         memcpy(rxbuf, buf, size);
         memset(rxbuf + size, 0, sizeof(rxbuf) - size);
         rxbuf_ptr = rxbuf;
-- 
2.5.0

Re: [Qemu-devel] [PATCH] net: cadence_gem: check packet size in gem_recieve

[Jason Wang]

On 01/15/2016 03:00 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While receiving packets in 'gem_receive' routine, if Frame Check
> Sequence(FCS) is enabled, it copies the packet into a local
> buffer without checking its size. Add check to validate packet
> length against the buffer size to avoid buffer overflow.
>
> Reported-by: Ling Liu <liuling-it@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/cadence_gem.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3639fc1..c3a273b 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -677,10 +677,14 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      } else {
>          unsigned crc_val;
>  
> +        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
> +            size = sizeof(rxbuf) - sizeof(crc_val);
> +        }
> +        bytes_to_copy = size;
> +
>          /* The application wants the FCS field, which QEMU does not provide.
>           * We must try and calculate one.
>           */
> -

Unnecessary whitespace change.

>          memcpy(rxbuf, buf, size);

We probably need more check, is there any guarantee that size <= 2048?
If not, need fix.

Thanks

>          memset(rxbuf + size, 0, sizeof(rxbuf) - size);
>          rxbuf_ptr = rxbuf;

Re: [Qemu-devel] [PATCH] net: cadence_gem: check packet size in gem_recieve

[P J P]

+-- On Mon, 18 Jan 2016, Jason Wang wrote --+
| > +        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
| > +            size = sizeof(rxbuf) - sizeof(crc_val);
| > +        }
| > +        bytes_to_copy = size;
| > +
| 
| We probably need more check, is there any guarantee that size <= 2048?
| If not, need fix.

  Sorry? The above check would fix that, no?

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: cadence_gem: check packet size in gem_recieve

[Jason Wang]

On 01/18/2016 01:34 PM, P J P wrote:
> +-- On Mon, 18 Jan 2016, Jason Wang wrote --+
> | > +        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
> | > +            size = sizeof(rxbuf) - sizeof(crc_val);
> | > +        }
> | > +        bytes_to_copy = size;
> | > +
> | 
> | We probably need more check, is there any guarantee that size <= 2048?
> | If not, need fix.
>
>   Sorry? The above check would fix that, no?

You're right. Apply to my -net (and removing the unnecessary whitespace
change).

Thanks

> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F