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

[Atsushi Kumagai]

>>>> 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.
>>>
>
>Yes, all your comments are correct. :)
>So would you like I send out a v2 patch with all your comments in it?

I'd like to merge this patch into v1.5.6 since it's a critical issue,
so could you send the v2 patch?


Thanks
Atsushi Kumagai

>>> 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.
>>
>
>Correct, 2G >> 3 = 256M.
>
>
>> Thanks.
>> HATAYAMA, Daisuke
>>
>
>
>--
>Thanks,
>Jingbai Ma
>
>_______________________________________________
>kexec mailing list
>kexec at lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec