Patchwork cadence_gem: Avoid stack-writing buffer-overrun

login
register
mail settings
Submitter Peter A. G. Crosthwaite
Date June 19, 2012, 6:44 a.m.
Message ID <1340088278-8406-1-git-send-email-peter.crosthwaite@petalogix.com>
Download mbox | patch
Permalink /patch/165659/
State New
Headers show

Comments

Peter A. G. Crosthwaite - June 19, 2012, 6:44 a.m.
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(-)
Stefan Hajnoczi - June 22, 2012, 9:03 a.m.
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.
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.
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

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) {