Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ernestvh wants to merge 2 commits into
base: lf-6.12.y
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1986592
drivers: firmware: imx: fix dependency for IMX_SEC_ENCLAVE
MaxKrummenacher Sep 8, 2025
eb38225
drivers: firmware: imx: fix reference counting of se_kobj
Sep 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/firmware/imx/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ config IMX_SCMI_MISC_DRV

config IMX_SEC_ENCLAVE
tristate "i.MX Embedded Secure Enclave - EdgeLock Enclave Firmware driver."
depends on IMX_MBOX && ARCH_MXC && ARM64
depends on NVMEM_IMX_OCOTP_SCU && IMX_MBOX && ARCH_MXC && ARM64
Copy link
Contributor

@iuliana-prodan iuliana-prodan Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@ernestvh ernestvh Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Credit to @MaxKrummenacher!

Copy link
Contributor

@dbaluta dbaluta Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MaxKrummenacher MaxKrummenacher Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

select FW_LOADER
default m if ARCH_MXC

Expand Down
11 changes: 6 additions & 5 deletions drivers/firmware/imx/se_ctrl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2076,11 +2076,6 @@ static void se_if_probe_cleanup(void *plat_dev)
* un-set bit.
*/
of_reserved_mem_device_release(dev);

/* Free Kobj created for logging */
if (se_kobj)
kobject_put(se_kobj);

}

static int get_se_fw_img_nm_idx(const struct se_fw_img_name *se_fw_img_nm)
Expand Down Expand Up @@ -2280,6 +2275,8 @@ static int se_if_probe(struct platform_device *pdev)
ret = se_sysfs_log();
if (ret)
pr_warn("Warn: Creating sysfs entry for se_log and se_rcv_msg_timeout: %d\n", ret);
} else {
kobject_get(se_kobj);
Copy link
Contributor

@iuliana-prodan iuliana-prodan Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()).

Copy link
Author

@ernestvh ernestvh Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ernestvh ernestvh Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@iuliana-prodan iuliana-prodan Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

dev_info(dev, "i.MX secure-enclave: %s%d interface to firmware, configured.\n",
Expand All @@ -2296,6 +2293,10 @@ static int se_if_probe(struct platform_device *pdev)
static void se_if_remove(struct platform_device *pdev)
{
se_if_probe_cleanup(pdev);

/* Free Kobj created for logging */
if (se_kobj)
kobject_put(se_kobj);
}

static int se_suspend(struct device *dev)
Expand Down