ARM: kprobes: Fix false positive with FORTIFY_SOURCE

PATCH In review Kees Cook 4 participants 6 messages Oct 22, 2018 – Nov 1, 2018 Arm (32-bit)

Share & tools

Kees Cook

author

2018-10-22 09:30 UTC #0 lore raw

PATCH 1/1 ARM: kprobes: Fix false positive with FORTIFY_SOURCE

The arm compiler internally interprets an inline assembly label as an unsigned long value, not a pointer. As a result, under CONFIG_FORTIFY_SOURCE, the size of the array pointed to by an address of a label is 4 bytes, which was tripping the runtime checks. Instead, we can just cast the label (as done with the size calculations earlier) to avoid the problem.

Reported-by William Cohen <[email protected]>

Fixes 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")

Cc stable at vger.kernel.org

Signed-off-by Kees Cook <[email protected]>

---
 arch/arm/probes/kprobes/opt-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/probes/kprobes/opt-arm.c +1 -1

index b2aa9b32bff2..2c118a6ab358 100644
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
 	}
 
 	/* Copy arch-dep-instance from template. */
-	memcpy(code, &optprobe_template_entry,
+	memcpy(code, (unsigned char *)optprobe_template_entry,
 			TMPL_END_IDX * sizeof(kprobe_opcode_t));
 
 	/* Adjust buffer according to instruction. */

Signature · 6 lines

2.17.1


-- 
Kees Cook
Pixel Security

Laura Abbott

Acked-by on v1 1/1

2018-10-22 10:30 UTC #1 lore raw

Quoted 7 lines · Kees Cook

The arm compiler internally interprets an inline assembly label as an unsigned long value, not a pointer. As a result, under CONFIG_FORTIFY_SOURCE, the size of the array pointed to by an address of a label is 4 bytes, which was tripping the runtime checks. Instead, we can just cast the label (as done with the size calculations earlier) to avoid the problem.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1639397

Acked-by Laura Abbott <[email protected]>

Quoted 22 lines

Reported-by William Cohen <[email protected]>
Fixes 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc stable at vger.kernel.org
Signed-off-by Kees Cook <[email protected]>
---
  arch/arm/probes/kprobes/opt-arm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/probes/kprobes/opt-arm.c +1 -1
index b2aa9b32bff2..2c118a6ab358 100644
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
  	}
 
  	/* Copy arch-dep-instance from template. */
-	memcpy(code, &optprobe_template_entry,
+	memcpy(code, (unsigned char *)optprobe_template_entry,
  			TMPL_END_IDX * sizeof(kprobe_opcode_t));
 
  	/* Adjust buffer according to instruction. */

Masami Hiramatsu

Acked-by on v1 1/1

2018-10-23 01:06 UTC #2 lore raw

In reply to Kees Cook

Quoted 11 lines · On Mon, 22 Oct 2018 02:30:23 -0700 Kees Cook <[email protected]> wrote:

The arm compiler internally interprets an inline assembly label as an unsigned long value, not a pointer. As a result, under CONFIG_FORTIFY_SOURCE, the size of the array pointed to by an address of a label is 4 bytes, which was tripping the runtime checks. Instead, we can just cast the label (as done with the size calculations earlier) to avoid the problem.
Reported-by William Cohen <[email protected]>
Fixes 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc stable at vger.kernel.org
Signed-off-by Kees Cook <[email protected]>

Good catch! This looks good to me.

Acked-by Masami Hiramatsu <[email protected]>

Thank you,

Quoted 24 lines

---
 arch/arm/probes/kprobes/opt-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/probes/kprobes/opt-arm.c +1 -1

index b2aa9b32bff2..2c118a6ab358 100644
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
 	}
 
 	/* Copy arch-dep-instance from template. */
-	memcpy(code, &optprobe_template_entry,
+	memcpy(code, (unsigned char *)optprobe_template_entry,
 			TMPL_END_IDX * sizeof(kprobe_opcode_t));
 
 	/* Adjust buffer according to instruction. */

Signature · 6 lines

2.17.1


-- 
Kees Cook
Pixel Security

Signature · 1 line

Masami Hiramatsu <[email protected]>

William Cohen

2018-10-30 17:40 UTC #3 lore raw

In reply to Kees Cook

Quoted 21 lines · Kees Cook

The arm compiler internally interprets an inline assembly label as an unsigned long value, not a pointer. As a result, under CONFIG_FORTIFY_SOURCE, the size of the array pointed to by an address of a label is 4 bytes, which was tripping the runtime checks. Instead, we can just cast the label (as done with the size calculations earlier) to avoid the problem.
Reported-by William Cohen <[email protected]>
Fixes 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc stable at vger.kernel.org
Signed-off-by Kees Cook <[email protected]>
---
 arch/arm/probes/kprobes/opt-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/probes/kprobes/opt-arm.c +0 -0
index b2aa9b32bff2..2c118a6ab358 100644

arch/arm/probes/kprobes/opt-arm.c int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or · line 253

Quoted 8 lines

arch/arm/probes/kprobes/opt-arm.c +1 -1

@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
 	}
 
 	/* Copy arch-dep-instance from template. */
-	memcpy(code, &optprobe_template_entry,
+	memcpy(code, (unsigned char *)optprobe_template_entry,
 			TMPL_END_IDX * sizeof(kprobe_opcode_t));
 
 	/* Adjust buffer according to instruction. */

The patch fixes the issue for kretprobes. It looks good to me.

Thanks,

-Will

Masami Hiramatsu

Acked-by

2018-11-01 14:41 UTC #4 lore raw

Quoted 28 lines · On Tue, 30 Oct 2018 13:40:27 -0400 William Cohen <[email protected]> wrote:

Quoted 27 lines · Kees Cook
The arm compiler internally interprets an inline assembly label as an unsigned long value, not a pointer. As a result, under CONFIG_FORTIFY_SOURCE, the size of the array pointed to by an address of a label is 4 bytes, which was tripping the runtime checks. Instead, we can just cast the label (as done with the size calculations earlier) to avoid the problem.
Reported-by William Cohen <[email protected]>
Fixes 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc stable at vger.kernel.org
Signed-off-by Kees Cook <[email protected]>
---
 arch/arm/probes/kprobes/opt-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/probes/kprobes/opt-arm.c +1 -1
index b2aa9b32bff2..2c118a6ab358 100644
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
 	}
 
 	/* Copy arch-dep-instance from template. */
-	memcpy(code, &optprobe_template_entry,
+	memcpy(code, (unsigned char *)optprobe_template_entry,
 			TMPL_END_IDX * sizeof(kprobe_opcode_t));
 

Quoted 4 lines

arch/arm/probes/kprobes/opt-arm.c int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or · line 253
Quoted 2 lines
arch/arm/probes/kprobes/opt-arm.c +0 -0
 	/* Adjust buffer according to instruction. */
The patch fixes the issue for kretprobes. It looks good to me.

This looks good to me too. Thank you for finding and fixing it :)

Acked-by Masami Hiramatsu <[email protected]>

Thanks!

Quoted 4 lines

Thanks,
-Will

Signature · 1 line

Masami Hiramatsu <[email protected]>

Kees Cook

author

2018-11-01 20:53 UTC #5 lore raw

Quoted 36 lines · Masami Hiramatsu

Quoted 28 lines · On Tue, 30 Oct 2018 13:40:27 -0400 William Cohen <[email protected]> wrote:
Quoted 27 lines · Kees Cook
The arm compiler internally interprets an inline assembly label as an unsigned long value, not a pointer. As a result, under CONFIG_FORTIFY_SOURCE, the size of the array pointed to by an address of a label is 4 bytes, which was tripping the runtime checks. Instead, we can just cast the label (as done with the size calculations earlier) to avoid the problem.
Reported-by William Cohen <[email protected]>
Fixes 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc stable at vger.kernel.org
Signed-off-by Kees Cook <[email protected]>
---
 arch/arm/probes/kprobes/opt-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/probes/kprobes/opt-arm.c +1 -1
index b2aa9b32bff2..2c118a6ab358 100644
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
    }
 
    /* Copy arch-dep-instance from template. */
-   memcpy(code, &optprobe_template_entry,
+   memcpy(code, (unsigned char *)optprobe_template_entry,
                    TMPL_END_IDX * sizeof(kprobe_opcode_t));
 
Quoted 4 lines
arch/arm/probes/kprobes/opt-arm.c int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or · line 253
Quoted 2 lines
arch/arm/probes/kprobes/opt-arm.c +0 -0
    /* Adjust buffer according to instruction. */
The patch fixes the issue for kretprobes. It looks good to me.

Quoted 5 lines

This looks good to me too. Thank you for finding and fixing it :)
Acked-by Masami Hiramatsu <[email protected]>
Thanks!

Thanks for acks and testing. I've tossed this at the arm patch tracker:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8806/1

Signature · 1 line

Kees Cook