5/22/2020 Update: This kind of update should not have been necessary, but due to irresponsible journalists and the nature of social media, it is important to make some things perfectly clear:

Nowhere did we claim this was anything more than a trivially exploitable vulnerability. It is not a backdoor or an attempted backdoor, the term does not appear elsewhere in this blog at all; any suggestion of the sort was fabricated by irresponsible journalists who did not contact us and do not speak for us.

There is no chance this code would have passed review and be merged. No one can push or force code upstream.

This code is not characteristic of the quality of other code contributed upstream by Huawei. Contrary to baseless assertions from some journalists, this is not Huawei's first attempt at contributing to the kernel, in fact they've been a frequent contributor for some time.

5/11/2020 Update: We were contacted this morning by Huawei PSIRT who referenced an email by the patch author to the KSPP list: https://www.openwall.com/lists/kernel-hardening/2020/05/10/3 and stated that "The patchset is not provided by Huawei official but an individual. And also not used in any Huawei devices." They asked if we would update the description of the article to correct this information.

Based on publicly-available information, we know the author of the patch is a Huawei employee, and despite attempts now to distance itself from the code after publication of this post, it still retains the Huawei naming. Further, on information from our sources, the employee is a Level 20 Principal Security staffer, the highest technical level within Huawei.

The Github repository mentioned in the article had a commit added to it this morning that inserted a notice to the top of the README file, distancing the code from Huawei. This commit was (intentionally or not) backdated to Friday when the repository was created, creating the impression that we somehow intentionally ignored pertinent information that was readily available. This is obviously untrue, and examining the contents of https://api.github.com/repos/cloudsec/hksp/events proves the commit was pushed to the repo this morning.

We replied to Huawei PSIRT's mail and mentioned that we'd be fine with mentioning the patches aren't shipping on any Huawei devices (I believed it already to be unlikely given the poor code quality), but regarding the other claim (particularly due to the surreptitious Github repo edit), we'd have to also include the additional information we discovered.

Original post continues below:

Huawei has seemingly stepped its foot into the kernel-self protection game with the release of HKSP. The patch itself is riddled with bugs and weaknesses and generally lacks any kind of threat model (making its mitigations similar to those present in LKRG where knowledge of the mitigation in place is enough to bypass it). It is not clear if the posted patchset is an official Huawei release or whether this code is already shipping on any Huawei devices, but the patchset uses Huawei in its name, and the Github account for the patchset lists Huawei as the organization for the account.

Our focus today in this short blog post will be how the HKSP patchset introduced a trivially exploitable vulnerability due to a complete lack of defensive programming.

In the patch, a /proc/ksguard/state entry is created. Giving a hint to the level of review of the code, every time this entry is opened or closed, the following lines referencing a nonexistent filename are output to dmesg:
open /proc/ksg_state_proc ok.
close /proc/ksg_state_proc ok.

As we can see in the below line of the patch, the state entry is created with global RWX rights, a sign of carelessness given that the entry doesn't support any meaningful read operation and execute is meaningless on such an entry.

state_proc = proc_create("state", 0777, ksg_proc, &ksg_state_ops);

The ksg_state_write handler for writes to this entry looks like this:

static ssize_t ksg_state_write(struct file *file, const char __user *buf,
                      size_t len, loff_t *offset)
{
	u64 value;
	char tmp[32];
	size_t n = 0;

        if (copy_from_user(tmp, buf, len))
                return -1;

	value = simple_strtoul(tmp, '\0', 10);
	switch (value) {
	case 1:
		ksg_check_keyboard();
		break;
	case 2:
		ksg_check_nf();
		break;
	case 3:
		ksg_check_pointer();
		break;
	case 4:
		ksg_check_sct();
		break;
	default:
		break;
	}

        *offset += len;
        n += len;

        return len;
}

There are a number of issues with this function. First, there is the return -1 which should return a proper errno (generally -EFAULT). There is an n variable that is assigned to, but not used for anything. There are no checks at all on the value of len. The first issue this causes is acting as a limited oracle. By writing 0 bytes to the entry, the uninitialized tmp array will attempt to have a number parsed out of it and then acted upon. Likewise, writing a full 32 bytes to the entry and failing to NUL-terminate the string will cause simple_strtoul to access out of bounds, potentially operating as a partial oracle for adjacent memory. Most importantly, due to the lack of checks on len, and given that tmp is a simple 32-byte stack array, this introduces a trivially exploitable kernel stack buffer overflow able to be performed by any unprivileged user.

For completeness, a simple PoC is provided below:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

int main(void)
{
        char buf[4096] = { };
        int fd = open("/proc/ksguard/state", O_WRONLY);
        if (fd >= 0) {
		write(fd, buf, sizeof(buf));
		close(fd);
	}
        return 0;
}

Effective security defenses require defined, realistic threat models. Defenses in the kernel should be programmed defensively and with reducing maintenance burdens in mind. The kernel can effectively be thought of as the largest, most vulnerable setuid root binary on the system. New code added to this most-privileged component of the system is potential new attack surface and requires heavy scrutiny, lest worse problems be introduced than were attempted to be solved in the first place.