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

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

[Jingbai Ma]

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
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));
 }


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

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

[Petr Tesarik]

On Thu, 17 Apr 2014 15:59:02 +0800
Jingbai Ma <jingbai.ma@hp.com> 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
> 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.

While the patch is correct, the explanation is not precise, because it
does not explain why an unsigned long is not sufficient. In fact, it
would be enough on 64-bit systems, where an unsigned long is just as big
as an unsigned long long (both 64 bits). But on 32-bit systems, an
unsigned long is 32-bit, but physical memory can be larger (e.g. 36
bits with PAE).

Petr T

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

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

[HATAYAMA, Daisuke]

(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