[PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary

[HATAYAMA Daisuke]

From: HATAYAMA Daisuke <d.hatayama at jp.fujitsu.com>
Subject: Re: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
Date: Tue, 15 Apr 2014 10:15:21 +0900

> From: Jingbai Ma <jingbai.ma at hp.com>
> Subject: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
> Date: Mon, 14 Apr 2014 18:35:44 +0800
> 
>> This patch intend to fix a segmentation fault when pfn exceeds 2G boundary.
>> 
>> In function is_on(), if the pfn (i) is greater than 2G, it will be a negative
>> value and will cause a segmentation fault.
>> is_on(char *bitmap, int i)
>> {
>>         return bitmap[i>>3] & (1 << (i & 7));
>> }
>> 
>> 
>> Signed-off-by: Jingbai Ma <jingbai.ma at hp.com>
>> ---
>>  makedumpfile.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/makedumpfile.h b/makedumpfile.h
>> index 3d270c6..03d35a8 100644
>> --- a/makedumpfile.h
>> +++ b/makedumpfile.h
>> @@ -1591,7 +1591,7 @@ int get_xen_info_ia64(void);
>>  #endif	/* s390x */
>>  
>>  static inline int
>> -is_on(char *bitmap, int i)
>> +is_on(char *bitmap, unsigned long long i)
>>  {
>>  	return bitmap[i>>3] & (1 << (i & 7));
>>  }
>> 
> 
> is_on is used at the following two places.
> 
> static inline int
> is_dumpable(struct dump_bitmap *bitmap, unsigned long long pfn)
> {
>         off_t offset;
>         if (pfn == 0 || bitmap->no_block != pfn/PFN_BUFBITMAP) {
>                 offset = bitmap->offset + BUFSIZE_BITMAP*(pfn/PFN_BUFBITMAP);
>                 lseek(bitmap->fd, offset, SEEK_SET);
>                 read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP);
>                 if (pfn == 0)
>                         bitmap->no_block = 0;
>                 else
>                         bitmap->no_block = pfn/PFN_BUFBITMAP;
>         }
>         return is_on(bitmap->buf, pfn%PFN_BUFBITMAP);
> }
> 
> PFN_BUFBTIMAP is constant 32 K. So, it seems that the 4 byte byte
> length came here.
> 
> But right shift to signed integer is implementation defined. We should
> not use right shift to signed integer. it looks gcc performs
> arithmetic shift and this bahaviour is buggy in case of is_on().
> 
> static inline int
> is_dumpable_cyclic(char *bitmap, unsigned long long pfn, struct cycle *cycle)
> {
>         if (pfn < cycle->start_pfn || cycle->end_pfn <= pfn)
>                 return FALSE;
>         else
>                 return is_on(bitmap, pfn - cycle->start_pfn);
> }
> 
> Simply, (pfn - cycle->start_pfn) could be (info->max_mapnr - 0). It's
> possible to pass more than 2 Gi by using system with more than 8 TiB
> physical memory space.
> 
> So,
> 
> - i must be unsigned in order to make right shift operation
>   meaningful, and
> 
> - i must have 8 byte for systems with more than 8 TiB physical memory
>   space.
> 
> BTW, to make this situation happen in cyclic mode, there needs to be
> 16 GiB free memory for bitmap to become large enough. I guess you
> don't see this segmentation in the kdump 2nd kernel. Is this right?
> 

This is my terrible careless miss... 256 MiB is correct. 16 GiB is
obviously too large. Then, the amount seems typical.

Thanks.
HATAYAMA, Daisuke