[PATCH] libfdt: check for potential overrun in _fdt_splice()

[PATCH] libfdt: check for potential overrun in _fdt_splice()

[Bjorn Andersson]

From: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

Signed-off-by: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 libfdt/fdt_rw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 70adec6c371b..8be02b1f68f3 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void *splicepoint, int oldlen, int newlen)
 
 	if (((p + oldlen) < p) || ((p + oldlen) > end))
 		return -FDT_ERR_BADOFFSET;
+	if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+		return -FDT_ERR_BADOFFSET;
 	if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
 		return -FDT_ERR_NOSPACE;
 	memmove(p + newlen, p + oldlen, end - p - oldlen);
-- 
1.8.2.2

Re: [PATCH] libfdt: check for potential overrun in _fdt_splice()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

On Mon, Aug 10, 2015 at 11:39:02AM -0700, Bjorn Andersson wrote:
> From: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> 
> Signed-off-by: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>


The logic looks ok, but this needs a commit message explaining the
details of what condition it is protecting against, and how it might
arise.

> ---
>  libfdt/fdt_rw.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 70adec6c371b..8be02b1f68f3 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void *splicepoint, int oldlen, int newlen)
>  
>  	if (((p + oldlen) < p) || ((p + oldlen) > end))
>  		return -FDT_ERR_BADOFFSET;
> +	if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
> +		return -FDT_ERR_BADOFFSET;
>  	if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
>  		return -FDT_ERR_NOSPACE;
>  	memmove(p + newlen, p + oldlen, end - p - oldlen);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2] libfdt: check for potential overrun in _fdt_splice()

[Bjorn Andersson]

From: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

This patch catches the conditions where:
 - 'splicepoint' is set to a point outside of [ fdt, fdt_totalsize(fdt) )
 - 'newlen' is negative, or 'splicepoint' plus 'newlen' results in overflow

Either of these cases can be caused by math which overflows in calling
functions, or by sizes specified through dynamic means.

Signed-off-by: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 libfdt/fdt_rw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 70adec6c371b..8be02b1f68f3 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void *splicepoint, int oldlen, int newlen)
 
 	if (((p + oldlen) < p) || ((p + oldlen) > end))
 		return -FDT_ERR_BADOFFSET;
+	if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+		return -FDT_ERR_BADOFFSET;
 	if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
 		return -FDT_ERR_NOSPACE;
 	memmove(p + newlen, p + oldlen, end - p - oldlen);
-- 
2.4.2

Re: [PATCH v2] libfdt: check for potential overrun in _fdt_splice()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On Tue, Dec 01, 2015 at 04:43:10PM -0800, Bjorn Andersson wrote:
> From: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> 
> This patch catches the conditions where:
>  - 'splicepoint' is set to a point outside of [ fdt, fdt_totalsize(fdt) )
>  - 'newlen' is negative, or 'splicepoint' plus 'newlen' results in overflow
> 
> Either of these cases can be caused by math which overflows in calling
> functions, or by sizes specified through dynamic means.
> 
> Signed-off-by: Courtney Cavin <courtney.cavin-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

Applied, thanks.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]