diff mbox

cadence_gem: Avoid stack-writing buffer-overrun

Message ID 1340088278-8406-1-git-send-email-peter.crosthwaite@petalogix.com
State New
Headers show

Commit Message

Peter A. G. Crosthwaite June 19, 2012, 6:44 a.m. UTC 
From: Jim Meyering <meyering@redhat.com>

Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
of bytes to clear.  The latter would always clear 4 or 8
bytes, possibly writing beyond the end of that stack buffer.
Alternatively, depending on the value of the "size" parameter,
it could fail to initialize the end of "rxbuf".
Spotted by coverity.

Signed-off-by: Jim Meyering <meyering@redhat.com>
Signed-off-by: Peter A. G. Crosthwaite <peter.croshtwaite@petalogix.com>
---
 hw/cadence_gem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Stefan Hajnoczi June 22, 2012, 9:03 a.m. UTC | #1 
On Tue, Jun 19, 2012 at 04:44:38PM +1000, Peter A. G. Crosthwaite wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
> of bytes to clear.  The latter would always clear 4 or 8
> bytes, possibly writing beyond the end of that stack buffer.
> Alternatively, depending on the value of the "size" parameter,
> it could fail to initialize the end of "rxbuf".
> Spotted by coverity.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> Signed-off-by: Peter A. G. Crosthwaite <peter.croshtwaite@petalogix.com>
> ---
>  hw/cadence_gem.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan
Peter Maydell June 22, 2012, 9:09 a.m. UTC | #2 
On 22 June 2012 10:03, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jun 19, 2012 at 04:44:38PM +1000, Peter A. G. Crosthwaite wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
>> of bytes to clear.

> Thanks, applied to the trivial patches tree:
> https://github.com/stefanha/qemu/commits/trivial-patches

This patch is already in an outstanding arm-devs pullreq (possibly
due to confusion on my part); it would probably be better not to
put it in the trivial-patches tree I guess.

thanks and sorry for the confusion
-- PMM
Stefan Hajnoczi June 22, 2012, 9:30 a.m. UTC | #3 
On Fri, Jun 22, 2012 at 10:09 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 22 June 2012 10:03, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Jun 19, 2012 at 04:44:38PM +1000, Peter A. G. Crosthwaite wrote:
>>> From: Jim Meyering <meyering@redhat.com>
>>>
>>> Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
>>> of bytes to clear.
>
>> Thanks, applied to the trivial patches tree:
>> https://github.com/stefanha/qemu/commits/trivial-patches
>
> This patch is already in an outstanding arm-devs pullreq (possibly
> due to confusion on my part); it would probably be better not to
> put it in the trivial-patches tree I guess.

Okay, I've dropped it.

Stefan
diff mbox

Patch

diff --git a/hw/cadence_gem.c b/hw/cadence_gem.c
index e2140ae..dbde392 100644
--- a/hw/cadence_gem.c
+++ b/hw/cadence_gem.c
@@ -664,7 +664,7 @@  static ssize_t gem_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
          */
 
         memcpy(rxbuf, buf, size);
-        memset(rxbuf + size, 0, sizeof(rxbuf - size));
+        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
         rxbuf_ptr = rxbuf;
         crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
         if (size < 60) {