Date: Fri, 2 Aug 2013 13:27:35 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Rupesh Gujare <rupesh.gujare@atmel.com>
Cc: devel@linuxdriverproject.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] staging: ozwpan: Increase farewell report size.
Message-ID: <20130802102735.GB5051@mwanda>
References: <1375379102-18217-1-git-send-email-rupesh.gujare@atmel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1375379102-18217-1-git-send-email-rupesh.gujare@atmel.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1420
Lines: 38

On Thu, Aug 01, 2013 at 06:45:01PM +0100, Rupesh Gujare wrote:
> Farewell report size can be bigger than one byte, increase array
> size to accomodate maximum 32 bytes of farewell report.
> 

Gar...  No.  This is not right.

1) There is no check limiting the size to 32 and it could be up to
   253 bytes.

2) Use defines instead of magic numbers.

3) The oz_farewell struct is supposed to be a variable length struct
   but the variable part is put in the middle.  It doesn't make any
   sense to put the length of the variable size array after then end
   of the array because we can never find it again!  Put the
   variable size array at the end.  Make it a zero length array.
   u8 len;
   u8 report[0];

4) In oz_add_farewell() we do this:

	f = kmalloc(sizeof(struct oz_farewell) + len - 1, GFP_ATOMIC);

    The "- 1" refers to sizeof(f->report) but because it was a magic
    number then it was missed when the sizeof(f->report) changed.

5) In [patch 6/6] we set the ->len member.  But because it is at the
   end of a variable length array with no limit check the remote
   attacker can just rewrite it using the memcpy() on the next line.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/