[PATCH 0/4] hw_breakpoints: Fix a bunch of adress range fixes

[PATCH 0/4] hw_breakpoints: Fix a bunch of adress range fixes

[Frederic Weisbecker]

It was initially a single patch that Oleg sent me a few weeks ago. Thinking
about it I think it may need a stable backport even though it doesn't look
very dangerous, but just in case.

So I've splitted the patch in 4 different parts because it may need
backporting on different tree version for each arch concerned.

Of course we need to consolidate that range check somewhere but after the
stable backport.

Comments.


Thanks,
	Frederic
---

Oleg Nesterov (4):
      arm: Fix the hw_breakpoint range check
      x86: Fix the hw_breakpoint range check
      arm64: Fix the hw_breakpoint range check
      sh: Fix hw_breakpoint the range check


 arch/arm/kernel/hw_breakpoint.c   | 2 +-
 arch/arm64/kernel/hw_breakpoint.c | 2 +-
 arch/sh/kernel/hw_breakpoint.c    | 2 +-
 arch/x86/kernel/hw_breakpoint.c   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

[PATCH 1/4] arm: Fix the hw_breakpoint range check

[Frederic Weisbecker]

From: Oleg Nesterov <oleg@redhat.com>

arch_check_bp_in_kernelspace() tries to avoid the overflow and does 2
TASK_SIZE checks but it needs OR, not AND. Consider va = TASK_SIZE -1
and len = 2 case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Fixes: f81ef4a920c8e1af75adf9f15042c2daa49d3cb3
Cc: <stable@vger.kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/arm/kernel/hw_breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 3d44660..8b38001 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -465,7 +465,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	va = info->address;
 	len = get_hbp_len(info->ctrl.len);
 
-	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+	return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);
 }
 
 /*
-- 
1.8.3.1

[PATCH 2/4] x86: Fix the hw_breakpoint range check

[Frederic Weisbecker]

From: Oleg Nesterov <oleg@redhat.com>

arch_check_bp_in_kernelspace() tries to avoid the overflow and does 2
TASK_SIZE checks but it needs OR, not AND. Consider va = TASK_SIZE -1
and len = 2 case.

Note: TASK_SIZE doesn't look right at least on x86, I think it should
be replaced by TASK_SIZE_MAX.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Fixes: 0067f1297241ea567f2b22a455519752d70fcca9
Cc: <stable@vger.kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/x86/kernel/hw_breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index f66ff16..1131c1f 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -200,7 +200,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	va = info->address;
 	len = get_hbp_len(info->len);
 
-	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+	return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);
 }
 
 int arch_bp_generic_fields(int x86_len, int x86_type,
-- 
1.8.3.1

[PATCH 3/4] arm64: Fix the hw_breakpoint range check

[Frederic Weisbecker]

From: Oleg Nesterov <oleg@redhat.com>

arch_check_bp_in_kernelspace() tries to avoid the overflow and does 2
TASK_SIZE checks but it needs OR, not AND. Consider va = TASK_SIZE -1
and len = 2 case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Fixes: 478fcb2cdb2351dcfc3fb23f42d76f4436ee4149
Cc: <stable@vger.kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/arm64/kernel/hw_breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index ff516f6..4bbbfde 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -293,7 +293,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	va = info->address;
 	len = get_hbp_len(info->ctrl.len);
 
-	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+	return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);
 }
 
 /*
-- 
1.8.3.1

[PATCH 4/4] sh: Fix hw_breakpoint the range check

[Frederic Weisbecker]

From: Oleg Nesterov <oleg@redhat.com>

arch_check_bp_in_kernelspace() tries to avoid the overflow and does 2
TASK_SIZE checks but it needs OR, not AND. Consider va = TASK_SIZE -1
and len = 2 case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Fixes: 09a072947791088b88ae15111cf68fc5aaaf758d
Cc: <stable@vger.kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/sh/kernel/hw_breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/sh/kernel/hw_breakpoint.c b/arch/sh/kernel/hw_breakpoint.c
index f917376..9801674 100644
--- a/arch/sh/kernel/hw_breakpoint.c
+++ b/arch/sh/kernel/hw_breakpoint.c
@@ -132,7 +132,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	va = info->address;
 	len = get_hbp_len(info->len);
 
-	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+	return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);
 }
 
 int arch_bp_generic_fields(int sh_len, int sh_type,
-- 
1.8.3.1

Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check

[Borislav Petkov]

On Sun, Nov 24, 2013 at 11:32:49AM +0100, Frederic Weisbecker wrote:
> From: Oleg Nesterov <oleg@redhat.com>
> 
> arch_check_bp_in_kernelspace() tries to avoid the overflow and does 2
> TASK_SIZE checks but it needs OR, not AND. Consider va = TASK_SIZE -1
> and len = 2 case.
> 
> Note: TASK_SIZE doesn't look right at least on x86, I think it should
> be replaced by TASK_SIZE_MAX.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Fixes: 0067f1297241ea567f2b22a455519752d70fcca9
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  arch/x86/kernel/hw_breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index f66ff16..1131c1f 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -200,7 +200,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
>  	va = info->address;
>  	len = get_hbp_len(info->len);
>  
> -	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
> +	return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);

Well, can't you simplify it even further?

	return (va + len - 1) >= TASK_SIZE;

AFAICT, the high end of the range matters, no?

Unless the original code was meant to short-circuit at the first
comparison already...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check

[Oleg Nesterov]

Frederic. Thanks for doing this ;)

On 11/24, Borislav Petkov wrote:
>
> On Sun, Nov 24, 2013 at 11:32:49AM +0100, Frederic Weisbecker wrote:
> >
> > -	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
> > +	return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);
>
> Well, can't you simplify it even further?
>
> 	return (va + len - 1) >= TASK_SIZE;

This won't work if va + len overflows?

Perhaps we should makes this clear, and we can even check the overflow
in the generic code (iirc Linus suggested to do this).

But to me it would be better to add the generic helper, they all do
the same check. Even arch/powerpc/kernel/hw_breakpoint.c whch doesn't
look right. Or make it __weak, or turn it into
arch_check_bp_in_kernelspace(start, end).

Oleg.

Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check

[Borislav Petkov]

On Mon, Nov 25, 2013 at 08:50:28PM +0100, Oleg Nesterov wrote:
> This won't work if va + len overflows?

Oh, right,

> Perhaps we should makes this clear, and we can even check the overflow
> in the generic code (iirc Linus suggested to do this).

maybe something like

	((va + len - 1) >= TASK_SIZE) || ((va + len - 1) < va)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check

[Oleg Nesterov]

On 11/25, Borislav Petkov wrote:
>
> On Mon, Nov 25, 2013 at 08:50:28PM +0100, Oleg Nesterov wrote:
> > This won't work if va + len overflows?
>
> Oh, right,
>
> > Perhaps we should makes this clear, and we can even check the overflow
> > in the generic code (iirc Linus suggested to do this).
>
> maybe something like
>
> 	((va + len - 1) >= TASK_SIZE) || ((va + len - 1) < va)

Yes. But again, it makes sense to do this in the caller. And kill
the stupid get_hbp_len(). And other cleanups.

But this patch just tries to fix the typo in the security check.

Oleg.