Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
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. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
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. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
On Sun, Nov 24, 2013 at 11:32:49AM +0100, Frederic Weisbecker wrote: > From: Oleg Nesterov > > 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 > Fixes: 0067f1297241ea567f2b22a455519752d70fcca9 > Cc: > Signed-off-by: Frederic Weisbecker > --- > 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. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] x86: Fix the hw_breakpoint range check
From: Oleg Nesterov 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 Fixes: 0067f1297241ea567f2b22a455519752d70fcca9 Cc: Signed-off-by: Frederic Weisbecker --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] x86: Fix the hw_breakpoint range check
From: Oleg Nesterov o...@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 o...@redhat.com Fixes: 0067f1297241ea567f2b22a455519752d70fcca9 Cc: sta...@vger.kernel.org Signed-off-by: Frederic Weisbecker fweis...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] x86: Fix the hw_breakpoint range check
On Sun, Nov 24, 2013 at 11:32:49AM +0100, Frederic Weisbecker wrote: From: Oleg Nesterov o...@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 o...@redhat.com Fixes: 0067f1297241ea567f2b22a455519752d70fcca9 Cc: sta...@vger.kernel.org Signed-off-by: Frederic Weisbecker fweis...@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. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
