[BUG]memblock: fix overflow of array index

[BUG]memblock: fix overflow of array index

[Peter Teoh]

Fixing the mismatch in signed and unsigned type assignment, which
potentially can lead to integer overflow bug.

Thanks.

Reviewed-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Peter Teoh <htmldeveloper@gmail.com>

diff --git a/mm/memblock.c b/mm/memblock.c
index a44eab3..2c621c5 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -553,8 +553,8 @@ void __init_memblock __next_free_mem_range(u64
*idx, int nid,
 {
      struct memblock_type *mem = &memblock.memory;
      struct memblock_type *rsv = &memblock.reserved;
-       int mi = *idx & 0xffffffff;
-       int ri = *idx >> 32;
+       unsigned int mi = *idx & 0xffffffff;
+       unsigned int ri = *idx >> 32;

      for ( ; mi < mem->cnt; mi++) {
              struct memblock_region *m = &mem->regions[mi];



--
Regards,
Peter Teoh

Re: [BUG]memblock: fix overflow of array index

[Tejun Heo]

On Wed, Apr 25, 2012 at 04:30:19PM +0800, Peter Teoh wrote:
> Fixing the mismatch in signed and unsigned type assignment, which
> potentially can lead to integer overflow bug.
> 
> Thanks.
> 
> Reviewed-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Peter Teoh <htmldeveloper@gmail.com>

All indexes in memblock are integers.  Changing that particular one to
unsigned int doesn't fix anything.  I think it just makes things more
confusing.  If there ever are cases w/ more then 2G memblocks, we're
going for 64bit not unsigned.

Thanks.

-- 
tejun

Re: [BUG]memblock: fix overflow of array index

[H. Peter Anvin]

On 04/25/2012 03:28 PM, Tejun Heo wrote:
> 
> All indexes in memblock are integers.  Changing that particular one to
> unsigned int doesn't fix anything.  I think it just makes things more
> confusing.  If there ever are cases w/ more then 2G memblocks, we're
> going for 64bit not unsigned.
> 

I would expect there to be plenty of memblocks larger than 2G?

	-hpa

Re: [BUG]memblock: fix overflow of array index

[David Miller]

From: "H. Peter Anvin" <hpa@linux.intel.com>
Date: Wed, 25 Apr 2012 15:29:31 -0700

> On 04/25/2012 03:28 PM, Tejun Heo wrote:
>> 
>> All indexes in memblock are integers.  Changing that particular one to
>> unsigned int doesn't fix anything.  I think it just makes things more
>> confusing.  If there ever are cases w/ more then 2G memblocks, we're
>> going for 64bit not unsigned.
>> 
> 
> I would expect there to be plenty of memblocks larger than 2G?

Yes, but not the array indexes into those memblock entries, which is
what this patch is about.

Re: [BUG]memblock: fix overflow of array index

[H. Peter Anvin]

On 04/25/2012 03:31 PM, David Miller wrote:
> From: "H. Peter Anvin" <hpa@linux.intel.com>
> Date: Wed, 25 Apr 2012 15:29:31 -0700
> 
>> On 04/25/2012 03:28 PM, Tejun Heo wrote:
>>>
>>> All indexes in memblock are integers.  Changing that particular one to
>>> unsigned int doesn't fix anything.  I think it just makes things more
>>> confusing.  If there ever are cases w/ more then 2G memblocks, we're
>>> going for 64bit not unsigned.
>>>
>>
>> I would expect there to be plenty of memblocks larger than 2G?
> 
> Yes, but not the array indexes into those memblock entries, which is
> what this patch is about.

OIC... more than 2^31 memblocks.

	-hpa

Re: [BUG]memblock: fix overflow of array index

[Peter Teoh]

Thanks for the reply.   Just an educational question:  is it possible
to set one-byte per memblock?    And what is the minimum memblock
size?

Even if 2G memblock is a huge number, it still seemed like a bug to me
that there is no check on the maximum number (which is 2G) of this
variable (assuming signed int).   Software can always purposely push
that number up and the system can panic?

On Thu, Apr 26, 2012 at 6:28 AM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Apr 25, 2012 at 04:30:19PM +0800, Peter Teoh wrote:
>> Fixing the mismatch in signed and unsigned type assignment, which
>> potentially can lead to integer overflow bug.
>>
>> Thanks.
>>
>> Reviewed-by: Minchan Kim <minchan@kernel.org>
>> Signed-off-by: Peter Teoh <htmldeveloper@gmail.com>
>
> All indexes in memblock are integers.  Changing that particular one to
> unsigned int doesn't fix anything.  I think it just makes things more
> confusing.  If there ever are cases w/ more then 2G memblocks, we're
> going for 64bit not unsigned.
>
> Thanks.
>
> --
> tejun



-- 
Regards,
Peter Teoh

Re: [BUG]memblock: fix overflow of array index

[Yinghai Lu]

On Wed, Apr 25, 2012 at 5:50 PM, Peter Teoh <htmldeveloper@gmail.com> wrote:
> Thanks for the reply.   Just an educational question:  is it possible
> to set one-byte per memblock?    And what is the minimum memblock
> size?

yes. 1 byte.

>
> Even if 2G memblock is a huge number, it still seemed like a bug to me
> that there is no check on the maximum number (which is 2G) of this
> variable (assuming signed int).   Software can always purposely push
> that number up and the system can panic?

before slab is ready? how?

Thanks

Yinghai

Re: [BUG]memblock: fix overflow of array index

[Tejun Heo]

Hello,

On Thu, Apr 26, 2012 at 08:50:58AM +0800, Peter Teoh wrote:
> Thanks for the reply.   Just an educational question:  is it possible
> to set one-byte per memblock?    And what is the minimum memblock
> size?

1 byte.

> Even if 2G memblock is a huge number, it still seemed like a bug to me
> that there is no check on the maximum number (which is 2G) of this
> variable (assuming signed int).   Software can always purposely push
> that number up and the system can panic?

Yeah, if somebody messes the BIOS / firmware to oblivion.  I don't
really care at that point tho.  memblock is a boot time memory
allocator and it assumes BIOS / firmware isn't completely crazy.  It
uses contiguous tables to describe all the blocks, walks them
one-by-one for allocation and even compacts them.

Well before memblock fails from any of the above, the machine would be
failing miserably in firmware / BIOS.

Thanks.

-- 
tejun