linux-kernel - Re: [PATCH] staging: wilc100: Remove pointer and integer comparision

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 10 Aug 2015 19:27:13 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Chandra Gorentla <csgorentla@...il.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
Subject: Re: [PATCH] staging: wilc100: Remove pointer and integer comparision

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
--
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/