CFI Type Mismatch (Function Pointer Cast) in ARM Linux Parser

[mhansen]

Hi, I spotted this when trying to run an Android application under UBSan. Upon digging, I found this issue:

A Clang Control Flow Integrity (CFI) type mismatch occurs in src/arm/linux/cpuinfo.c when invoking the parse_line callback through the cpuinfo_line_callback function pointer. This triggers a runtime SIGTRAP when CFI is enabled.

The issue is that parse_line is defined with a specific structure pointer as its third argument, but it is cast to cpuinfo_line_callback which expects void* for that argument.

In src/arm/linux/cpuinfo.c we have parse_line which takes a proc_cpuinfo_parser_state argument

cpuinfo/src/arm/linux/cpuinfo.c

Lines 726 to 730 in ae54436

	static bool parse_line(
	const char* line_start,
	const char* line_end,
	struct proc_cpuinfo_parser_state state[restrict static 1],
	uint64_t line_number) {

In cpuinfo_arm_linux_parse_proc_cpuinfo:

cpuinfo/src/arm/linux/api.h

Lines 297 to 301 in ae54436

	CPUINFO_INTERNAL bool cpuinfo_arm_linux_parse_proc_cpuinfo(
	char hardware[restrict static CPUINFO_HARDWARE_VALUE_MAX],
	char revision[restrict static CPUINFO_REVISION_VALUE_MAX],
	uint32_t max_processors_count,
	struct cpuinfo_arm_linux_processor processors[restrict static max_processors_count]);

The callback type is defined in src/linux/api.h as:

cpuinfo/src/linux/api.h

Line 37 in ae54436

typedef bool (*cpuinfo_line_callback)(const char*, const char*, void*, uint64_t);

The problem is specifically here, where we cast parse_line to cpuinfo_line_callback. These functions have different signatures:

cpuinfo/src/arm/linux/cpuinfo.c

Lines 1021 to 1023 in ae54436

	return cpuinfo_linux_parse_multiline_file(
	"/proc/cpuinfo", BUFFER_SIZE, (cpuinfo_line_callback)parse_line, &state);
	}

Calling a function through a pointer of a different type (even if they are pointer-compatible in some ABIs) is Undefined Behavior in C and is flagged by Clang's -fsanitize=cfi-icall.

Suggested fix: Change parse_line to accept void* for the context argument, and cast it inside the function. Remove the cast when passing it to cpuinfo_linux_parse_multiline_file.

--- a/src/arm/linux/cpuinfo.c
+++ b/src/arm/linux/cpuinfo.c
@@ -726,8 +726,9 @@
 static bool parse_line(
 	const char* line_start,
 	const char* line_end,
-	struct proc_cpuinfo_parser_state state[restrict static 1],
+	void* context,
 	uint64_t line_number) {
+	struct proc_cpuinfo_parser_state* restrict state = (struct proc_cpuinfo_parser_state*)context;
 	/* Empty line. Skip. */
 	if (line_start == line_end) {
 		return true;
@@ -1019,5 +1018,5 @@
 		.processors = processors,
 	};
 	return cpuinfo_linux_parse_multiline_file(
-		"/proc/cpuinfo", BUFFER_SIZE, (cpuinfo_line_callback)parse_line, &state);
+		"/proc/cpuinfo", BUFFER_SIZE, parse_line, &state);
 }

[mhansen]

I'm making a repro case, here's the stack we get:

third_party/cpuinfo/src/linux/multiline.c:85:11: runtime error: control flow integrity check for type 'bool (const char *, const char *, void *, unsigned long)' failed during indirect function call
third_party/cpuinfo/src/arm/linux/cpuinfo.c:730: note: parse_line defined here
    #0 0xaaaab8996e78 in cpuinfo_linux_parse_multiline_file third_party/cpuinfo/src/linux/multiline.c:85:11
    #1 0xaaaab8996e78 in cpuinfo_arm_linux_parse_proc_cpuinfo third_party/cpuinfo/src/arm/linux/cpuinfo.c:1021:9
    #2 0xaaaab8996a68 in main repro_cpuinfo_cast.c:26:3

Here's the code in google's bazel repo; I'll try and convert this for external soon

cc_test(
    name = "repro_cpuinfo_cast",
    srcs = ["repro_cpuinfo_cast.c"],
    linkstatic = True,
    copts = [
        "-O0",
        "-DCPUINFO_MOCK=1",
        "-Wno-vla",
        "-Ithird_party/cpuinfo/src",
        "-Ithird_party/cpuinfo/include",
    ],
    deps = [
        "//third_party/cpuinfo:cpuinfo_mock",
    ],
)

#include <stdio.h>
#include <string.h>
#include <cpuinfo-mock.h>
#include <arm/linux/api.h>

static struct cpuinfo_mock_file mock_files[] = {
    {
        .path = "/proc/cpuinfo",
        .size = 0, // Will be initialized in main
        .content =
            "Hardware\t: NormalHardware\n",
    },
    {.path = NULL}};

int main() {
  mock_files[0].size = strlen(mock_files[0].content);
  cpuinfo_mock_filesystem(mock_files);

  char hardware[64];
  char revision[9];
  struct cpuinfo_arm_linux_processor processors[1];

  printf("Calling cpuinfo_arm_linux_parse_proc_cpuinfo...\n");
  // This calls the parser, which has the bad cast.
  // If compiled with CFI, it should crash.
  cpuinfo_arm_linux_parse_proc_cpuinfo(hardware, revision, 1, processors);

  printf("Hardware parsed: %s\n", hardware);
  return 0;
}

blaze test \
  --cpu=arm \
  --config=ubsan \
  --copt=-fsanitize=cfi \
  --copt=-fvisibility=default \
  --copt=-fno-sanitize-trap=cfi \
  --linkopt=-fsanitize=cfi \
  --copt=-flto \
  --linkopt=-flto \
  :repro_cpuinfo_cast

[mhansen]

I see that you don't have any asan or ubsan or cfi tests on github actions. I wonder if there'd be any apetite to add, or if that'll just burn through your CI quota and it'd be undesired.

[malfet]

@mhansen if you want to add ASAN please do not hesitate to propose a PR, as you can see traffic to the repo is not that high, unless you anticipate that ASAN tests will be much longer