Re: xen/mmu: Copy and revector the P2M tree.

Re: xen/mmu: Copy and revector the P2M tree.

[Dan Carpenter]

Hello Konrad Rzeszutek Wilk,

The patch 7f9140626c75: "xen/mmu: Copy and revector the P2M tree."
from Jul 26, 2012, leads to the following static checker warning:

	arch/x86/xen/mmu.c:1105 xen_cleanhighmap()
	warn: potential pointer math issue ('level2_kernel_pgt' is pointer to unsigned long)

arch/x86/xen/mmu.c
  1096  #ifdef CONFIG_X86_64
  1097  static void __init xen_cleanhighmap(unsigned long vaddr,
  1098                                      unsigned long vaddr_end)
  1099  {
  1100          unsigned long kernel_end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1;
  1101          pmd_t *pmd = level2_kernel_pgt + pmd_index(vaddr);
  1102  
  1103          /* NOTE: The loop is more greedy than the cleanup_highmap variant.
  1104           * We include the PMD passed in on _both_ boundaries. */
  1105          for (; vaddr <= vaddr_end && (pmd < (level2_kernel_pgt + PAGE_SIZE));
                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This pointer math is weird because we typically think of PAGE_SIZE as
a number of bytes but since level2_kernel_pgt is a pointer to unsigned
long, it looks like this loop can go through more iterations than
intended.

  1106                          pmd++, vaddr += PMD_SIZE) {
  1107                  if (pmd_none(*pmd))
  1108                          continue;
  1109                  if (vaddr < (unsigned long) _text || vaddr > kernel_end)
  1110                          set_pmd(pmd, __pmd(0));
  1111          }
  1112          /* In case we did something silly, we should crash in this function
  1113           * instead of somewhere later and be confusing. */
  1114          xen_mc_flush();
  1115  }

regards,
dan carpenter

Re: xen/mmu: Copy and revector the P2M tree.

[Dan Carpenter]

On Mon, Jul 20, 2015 at 01:03:52PM +0300, Dan Carpenter wrote:
> Hello Konrad Rzeszutek Wilk,
> 
> The patch 7f9140626c75: "xen/mmu: Copy and revector the P2M tree."
> from Jul 26, 2012, leads to the following static checker warning:
> 
> 	arch/x86/xen/mmu.c:1105 xen_cleanhighmap()
> 	warn: potential pointer math issue ('level2_kernel_pgt' is pointer to unsigned long)
> 
> arch/x86/xen/mmu.c
>   1096  #ifdef CONFIG_X86_64
>   1097  static void __init xen_cleanhighmap(unsigned long vaddr,
>   1098                                      unsigned long vaddr_end)
>   1099  {
>   1100          unsigned long kernel_end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1;
>   1101          pmd_t *pmd = level2_kernel_pgt + pmd_index(vaddr);
>   1102  
>   1103          /* NOTE: The loop is more greedy than the cleanup_highmap variant.
>   1104           * We include the PMD passed in on _both_ boundaries. */
>   1105          for (; vaddr <= vaddr_end && (pmd < (level2_kernel_pgt + PAGE_SIZE));
>                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This pointer math is weird because we typically think of PAGE_SIZE as
> a number of bytes but since level2_kernel_pgt is a pointer to unsigned
> long, it looks like this loop can go through more iterations than
> intended.
> 

Yeah.  This condition doesn't make sense at all.  level2_kernel_pgt is
an array that has 512 elements but PAGE_SIZE is 4096 so we would be well
past the end of the array.

I suspect that this works because it is only called when DEBUG in
defined or because of the "vaddr <= vaddr_end" condition.

>   1106                          pmd++, vaddr += PMD_SIZE) {
>   1107                  if (pmd_none(*pmd))
>   1108                          continue;
>   1109                  if (vaddr < (unsigned long) _text || vaddr > kernel_end)
>   1110                          set_pmd(pmd, __pmd(0));
>   1111          }
>   1112          /* In case we did something silly, we should crash in this function
>   1113           * instead of somewhere later and be confusing. */
>   1114          xen_mc_flush();
>   1115  }
> 
> regards,
> dan carpenter