Re: [PATCH v2] makedumpfile: fix a segmentation fault when physical address exceeds 8TB boundary

["HATAYAMA, Daisuke" <d.hatayama@jp.fujitsu.com>]

(2014/04/17 16:59), Jingbai Ma wrote:
> This patch intends to fix a segmentation fault when physical address exceeds
> 8TB boundary.
>
> Changelog:
> v2:
> - Add more comments from Daisuke HATAYAMA.
>
>
> In function is_on(), if the physical address higher than 8T, pfn (i) will
> 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));
> }
>
> Daisuke's detailed analysis:
> 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

More precisely, here should have been ((info->max_mapnr - 1) - 0) since info->max_mapnr doesn't belong to target physical memory space...

> possible to pass more than 2 Gi by using system with more than 8 TiB
> physical memory space.
>
> So, in function is_on()
>
> - 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.
>
>
> Signed-off-by: Jingbai Ma <jingbai.ma@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));
>   }
>

-- 
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec