[PATCH] asm/sections: Check for overflow in memory_contains()

[PATCH] asm/sections: Check for overflow in memory_contains()

[Vincent Whitchurch]

ARM uses memory_contains() from its stacktrace code via this function:

 static inline bool in_entry_text(unsigned long addr)
 {
 	return memory_contains(__entry_text_start, __entry_text_end,
 			       (void *)addr, 1);
 }

addr is taken from the stack and can be a completely invalid.  If addr
is 0xffffffff, there is an overflow in the pointer arithmetic in
memory_contains() and in_entry_text() incorrectly returns true.

Fix this by adding an overflow check.  The check is done on unsigned
longs to avoid undefined behaviour.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 include/asm-generic/sections.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index d1779d442aa5..e6e1b381c5df 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -105,7 +105,15 @@ static inline int arch_is_kernel_initmem_freed(unsigned long addr)
 static inline bool memory_contains(void *begin, void *end, void *virt,
 				   size_t size)
 {
-	return virt >= begin && virt + size <= end;
+	unsigned long membegin = (unsigned long)begin;
+	unsigned long memend = (unsigned long)end;
+	unsigned long objbegin = (unsigned long)virt;
+	unsigned long objend = objbegin + size;
+
+	if (objend < objbegin)
+		return false;
+
+	return objbegin >= membegin && objend <= memend;
 }
 
 /**
-- 
2.20.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] asm/sections: Check for overflow in memory_contains()

[Russell King - ARM Linux admin]

On Tue, Dec 17, 2019 at 11:22:38AM +0100, Vincent Whitchurch wrote:
> ARM uses memory_contains() from its stacktrace code via this function:
> 
>  static inline bool in_entry_text(unsigned long addr)
>  {
>  	return memory_contains(__entry_text_start, __entry_text_end,
>  			       (void *)addr, 1);
>  }
> 
> addr is taken from the stack and can be a completely invalid.  If addr
> is 0xffffffff, there is an overflow in the pointer arithmetic in
> memory_contains() and in_entry_text() incorrectly returns true.
> 
> Fix this by adding an overflow check.  The check is done on unsigned
> longs to avoid undefined behaviour.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
>  include/asm-generic/sections.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index d1779d442aa5..e6e1b381c5df 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -105,7 +105,15 @@ static inline int arch_is_kernel_initmem_freed(unsigned long addr)
>  static inline bool memory_contains(void *begin, void *end, void *virt,
>  				   size_t size)
>  {
> -	return virt >= begin && virt + size <= end;
> +	unsigned long membegin = (unsigned long)begin;
> +	unsigned long memend = (unsigned long)end;
> +	unsigned long objbegin = (unsigned long)virt;
> +	unsigned long objend = objbegin + size;
> +
> +	if (objend < objbegin)
> +		return false;
> +
> +	return objbegin >= membegin && objend <= memend;

Would merely changing to:

	return virt >= begin && virt <= end - size;

be sufficient ?  Is end - size possible to underflow?

>  }
>  
>  /**
> -- 
> 2.20.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] asm/sections: Check for overflow in memory_contains()

[Vincent Whitchurch]

On Tue, Dec 17, 2019 at 11:28:31AM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Dec 17, 2019 at 11:22:38AM +0100, Vincent Whitchurch wrote:
> > ARM uses memory_contains() from its stacktrace code via this function:
> > 
> >  static inline bool in_entry_text(unsigned long addr)
> >  {
> >  	return memory_contains(__entry_text_start, __entry_text_end,
> >  			       (void *)addr, 1);
> >  }
> > 
> > addr is taken from the stack and can be a completely invalid.  If addr
> > is 0xffffffff, there is an overflow in the pointer arithmetic in
> > memory_contains() and in_entry_text() incorrectly returns true.
> > 
> > Fix this by adding an overflow check.  The check is done on unsigned
> > longs to avoid undefined behaviour.
> > 
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> > ---
> >  include/asm-generic/sections.h | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > index d1779d442aa5..e6e1b381c5df 100644
> > --- a/include/asm-generic/sections.h
> > +++ b/include/asm-generic/sections.h
> > @@ -105,7 +105,15 @@ static inline int arch_is_kernel_initmem_freed(unsigned long addr)
> >  static inline bool memory_contains(void *begin, void *end, void *virt,
> >  				   size_t size)
> >  {
> > -	return virt >= begin && virt + size <= end;
> > +	unsigned long membegin = (unsigned long)begin;
> > +	unsigned long memend = (unsigned long)end;
> > +	unsigned long objbegin = (unsigned long)virt;
> > +	unsigned long objend = objbegin + size;
> > +
> > +	if (objend < objbegin)
> > +		return false;
> > +
> > +	return objbegin >= membegin && objend <= memend;
> 
> Would merely changing to:
> 
> 	return virt >= begin && virt <= end - size;
> 
> be sufficient ?  Is end - size possible to underflow?

Something like this would trigger an underflow and return an incorrect
result with that expression, wouldn't it?

 memory_contains((void *)0x0000, (void *)0x1000, (void *)0x0, 0x1001))

AFAICS no current callers actually send in an object size which is
larger than the size of the memory, but perhaps it's best to be
defensive?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel