drivers: firmware: imx: fix dependency for IMX_SEC_ENCLAVE and use-after-free

[ernestvh]

Hi, previously @MaxKrummenacher opened #31

We since noticed that there are still issues with this driver, and now have a second commit to fix these. Doing this I noticed that we still need Max's earlier fix, but for functional reasons. The attached stack trace in the original commit message from Max is seemingly not caused by the issue it fixes but by another one.

I have therefore attached both commits but edited Max's commit message.

Please let me know if additional info aside from the commit messages is needed, and thanks for taking the time to investigate this.

@dbaluta

[dbaluta]

@ernestvh Thanks for the PR.

We are currently working to define a process for accepting patches from outside into our internal linux-imx tree. In order to speed this up, please first create a Github Issue that describes your problem, setup and steps to reproduce. Then link it to this PR.

I have already sent this patches internally for review and will get back to you with the feedback.

[iuliana-prodan]

LGTM - depends on ensures the enclave driver can’t be y when NVMEM_IMX_OCOTP_SCU is m, avoiding probe defers.
Good catch!

[ernestvh]

Credit to @MaxKrummenacher!

[dbaluta]

Credit to @MaxKrummenacher!

@ernestvh add @MaxKrummenacher as Suggested-by or Reported-by to give credit.

Also, NVMEM_IMX_OCOTP_SCU is enabled only for SCU (System controller unit) based platforms right (like 8qxp/8qm)?

So it means that IMX_SEC_ENCLAVE is only available on this boards.

[MaxKrummenacher]

So it means that IMX_SEC_ENCLAVE is only available on this boards.

The current imx_v8_defconfig enables NVMEM_IMX_OCOTP_SCU, so IMX_SEC_ENCLAVE would be there for boards without an SCU.

But you are right the this seems not the proper or complete solution to forcing IMX_SEC_ENCLAVE=m when the OCOTP driver is m.

Assuming it to handle the =m vs. =y issue correctly probably the following would be better:

depends on (NVMEM_IMX_OCOTP_SCU || NVMEM_IMX_OCOTP_ELE ) && IMX_MBOX && ARCH_MXC && ARM64

[iuliana-prodan]

@ernestvh Just a heads-up: this isn’t really my area of expertise, so feel free to correct me if I’m off.

From what I can tell se_sysfs_log() always tries to create a new /sys/kernel/se kobject and its files. If it succeeds, it returns 0 (meaning it was just created). If it fails, it cleans up and returns a negative value.

There’s no success path for "already existed".
So if another probe calls it again, kobject_create_and_add("se", kernel_kobj) will likely fail due to the name already being used, and you’ll get an error.

Because of that, in this patch, calling kobject_get(se_kobj) when ret == 0 (success) will increase the refcount unnecessarily for the first device, which could cause a leak (unless the last remove does two put()).

[ernestvh]

Hi @iuliana-prodan, thanks for taking the time to review this.

Only the first probe should ever call se_sysfs_log and thus kobject_create_and_add, subsequent probes will always have se_kobj set to some value and thus if (!se_kobj) { ret = se_sysfs_log(); ... will not be executed, with one exception noted below. Instead, the later probes will do the kobject_get and afterwards a kobject_put in their remove methods.

Indeed, se_sysfs_log() tries to create this kobject and its files every time. If kobject_create_and_add fails, se_kobj is still a null pointer, leading the next probe to try se_sysfs_log again. When the call to kobject_create_and_add fails, kobject_put(se_kobj) will be executed, that does not seem right, since at that moment se_kobj is still null (EDIT: the kernel code always checks for null first inside kobject_put, so this is not an issue) . This is the only way that I see that se_sysfs_log could be executed more than once, since all other paths would lead to se_kobj not being null. So I do not see the potential for a leak here, but you did put your finger on a bug.

If either of the sysfs_create_file calls fail, the reference count is decremented, and se_kobj will probably be cleaned up since kobject_create_and_add will have lead to a reference count of 1, but se_kobj will not be reset to null. So the next probe will not execute se_sysfs_log and the creation of the files is never tried again. Possibly this is an issue. Instead, the next probe will do kobject_get, and it will later do a kobject_put in its remove function. I do not see an issue with the reference count there.

In short, hitting "already existed" seems impossible to me, but it does seem like we have some issues with the failure logic here, but that does not seem to be introduced by this patch. In any case, we would only hit such issues if the system is out of memory, so maybe it is not a big concern.

This logic is certainly confusing and I am also not an expert on this, so please feel free to correct me as well.

[ernestvh]

If I'm correct about the possible issue, a solution could be to call se_sysfs_log from the probe every time instead of conditionally, and then do something like this in there:

/* Exposing the variable se_log via sysfs to enable/disable logging */
static int  se_sysfs_log(void)
{
	int ret = 0;

	if (!se_kobj) {
		/* Create kobject "se" located under /sys/kernel */
		se_kobj = kobject_create_and_add("se", kernel_kobj);
		if (!se_kobj) {
			pr_warn("kobject creation failed\n");
			ret = -ENOMEM;
			/* N.B.: kobject_put will check for null and nothing will be done */
			goto out;
		}
	} else {
		kobject_get(se_kobj);
	}

	/* TODO: check if this file already exists first */
	/* Create file for the se_log attribute */
	if (sysfs_create_file(se_kobj, &se_log_attr.attr)) {
		pr_err("Failed to create se file\n");
		ret = -ENOMEM;
		goto out;
	}
	
	/* TODO: check if this file already exists first */
	/* Create file for the se_rcv_msg_timeout attribute */
	if (sysfs_create_file(se_kobj, &se_rcv_msg_timeout_attr.attr)) {
		pr_err("Failed to create se_rcv_msg_timeout file\n");
		ret = -ENOMEM;
		goto out;
	}

out:
	if (ret)
		kobject_put(se_kobj);
	return ret;
}

This way the usage is more in one place and easier to follow, and with some additional logic the creation of the files could be tried again if a probe is deferred because of out-of-memory issues.

[iuliana-prodan]

Good points @ernestvh!
Agreed that only the first probe actually calls se_sysfs_log(). The main bug I still see is that if a sysfs_create_file() fails, we put() but never reset se_kobj, so later probes will skip creation and end up with a stale pointer and missing files.

I like the idea of moving the create/get logic fully into se_sysfs_log(), it would make the flow clearer and fix the stale pointer case.

Probably best to hear from the driver owner on which pattern they want.
Thanks!

[ernestvh]

We are currently working to define a process for accepting patches from outside into our internal linux-imx tree. In order to speed this up, please first create a Github Issue that describes your problem, setup and steps to reproduce. Then link it to this PR.

I have already sent this patches internally for review and will get back to you with the feedback.

Hi @dbaluta, thanks for the info, I will keep it in mind for future patches. It seems like I am not permitted to create issues on this repo currently.