Message-ID: <20150810174034.GA14776@gcs-HP-Compaq-nx6320>
Date:	Mon, 10 Aug 2015 23:10:34 +0530
From:	Chandra Gorentla <csgorentla@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	gregkh@...uxfoundation.org, rachel.kim@...el.com,
	dean.lee@...el.com, chris.park@...el.com,
	devel@...verdev.osuosl.org, linux-wireless@...r.kernel.org,
	johnny.kim@...el.com, linux-kernel@...r.kernel.org,
	sudipm.mukherjee@...il.com, csgorentla@...il.com
Subject: Re: [PATCH] staging: wilc100: Remove pointer and integer comparision

On Mon, Aug 10, 2015 at 07:27:13PM +0300, Dan Carpenter wrote:
> On Mon, Aug 10, 2015 at 08:59:43PM +0530, Chandra Gorentla wrote:
> > I agree with your suggestion
> > that we need to take a broader look.  Please help in understanding
> > how does that broader look is suggesting that the patch is not
> > addressing a right problem.  The gcc version I am using is - 4.8.2.
> > 
> > In the later part of your reply - you felt that there may be a
> > case in which more than the allocated number of bytes may be
> > copied in to the memory pointed to by 'pu8CurrByte' and memory
> > may get corrupted.  From the code in the function I am not seeing
> > that happening.  In the beginning of the function, this pointer
> > variable is assigned a block of memory whose size is
> > '->u32HeadLen' + '->u32TailLen' + 16.
> > 
> > The function is copying 16 individual bytes to this memory;
> > a smaller block of memory of size '->u32HeadLen' is being copied;
> > and an another smaller block of memory of size '->u32TailLen' may
> > be copied based on a condition.  After this last copy, the
> > function increments the pointer by '->u32TailLen' irrespective
> > of last copy takes place or not.  Hence I am not seeing any
> > corruption of the memory.
> 
> It is an integer overflow.  Try the test.c file I'll include below.
> 
> > 
> > It looks like that the last increment is just an operation that
> > does no harm.  In addition to this the pointer variable
> > 'pu8CurrByte' is just a local variable and it is not being used
> > after the last increment operation of the pointer.
> 
> It's a pointer to allocated memory.  We call WILC_MALLOC().
> 
> This function allocates a buffer, then it copies memory over with the
> memcpy().  The "*pu8CurrByte++ = " statements are copying memory but
> doing endian conversion as well.  (This is not the correct way to do
> endian conversion).
> 
> > 
> > After making the change in this patch I just did a 'make'.
> > I do not have the hardware which this driver supports.  So at
> > this point of time, I cannot test your suggestions on wilc1000
> > hardware.  Is there any way I can test this driver code on a
> > old notebook computer?
> 
> Don't worry too much about testing this.  Just write small test programs
> to help you understand as much as possible.  We are good at reviewing so
> you aren't going to break the code.
> 
> #include <stdio.h>
> 
> int main(void)
> {
> 	unsigned int u32HeadLen;
> 	unsigned int u32TailLen;
> 	int s32ValueSize;
> 
> 	u32HeadLen = 1000;
> 	u32TailLen = -1U - 500 - 16 + 1;
> 	s32ValueSize = u32HeadLen + u32TailLen + 16;
> 
> 	printf("Allocating %d bytes.\n", s32ValueSize);
> 	printf("Copying %u bytes into a %d byte buffer will corrupt memory.\n",
> 		u32HeadLen, s32ValueSize);
> 
> 	return 0;
> }
> 
> regards,
> dan carpenter

If the incoming parameters '->u32HeadLen' and '->u32TailLen' are not correct,
they can cause corruption of memory.  Is it not a different problem what the
patch trying to fix?

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

Powered by blists - more mailing lists