* [PATCH] asm/sections: Check for overflow in memory_contains()
@ 2019-12-17 10:22 ` Vincent Whitchurch
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2019-12-17 10:22 UTC (permalink / raw)
To: akpm; +Cc: arnd, treding, linux-arm-kernel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] asm/sections: Check for overflow in memory_contains()
@ 2019-12-17 10:22 ` Vincent Whitchurch
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2019-12-17 10:22 UTC (permalink / raw)
To: akpm; +Cc: Vincent Whitchurch, treding, linux-arm-kernel, arnd, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] asm/sections: Check for overflow in memory_contains()
2019-12-17 10:22 ` Vincent Whitchurch
@ 2019-12-17 10:28 ` Russell King - ARM Linux admin
-1 siblings, 0 replies; 6+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-17 10:28 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: akpm, Vincent Whitchurch, treding, linux-arm-kernel, arnd, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm/sections: Check for overflow in memory_contains()
@ 2019-12-17 10:28 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 6+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-17 10:28 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: Vincent Whitchurch, arnd, linux-kernel, akpm, treding, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm/sections: Check for overflow in memory_contains()
2019-12-17 10:28 ` Russell King - ARM Linux admin
@ 2019-12-18 14:49 ` Vincent Whitchurch
-1 siblings, 0 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2019-12-18 14:49 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: akpm, treding, linux-arm-kernel, arnd, linux-kernel
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm/sections: Check for overflow in memory_contains()
@ 2019-12-18 14:49 ` Vincent Whitchurch
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2019-12-18 14:49 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: akpm, treding, arnd, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-18 14:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-17 10:22 [PATCH] asm/sections: Check for overflow in memory_contains() Vincent Whitchurch
2019-12-17 10:22 ` Vincent Whitchurch
2019-12-17 10:28 ` Russell King - ARM Linux admin
2019-12-17 10:28 ` Russell King - ARM Linux admin
2019-12-18 14:49 ` Vincent Whitchurch
2019-12-18 14:49 ` Vincent Whitchurch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.