Re: [PATCH v11 4/4] firmware: coreboot: Add CFR firmware attributes driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Memory leak of enum label string when it contains invalid characters.
--

--- Patch [4]: [PATCH v11 4/4] firmware: coreboot: Add CFR firmware attributes driver ---
commit ea61591b8dd6ac619938eac5285db7c41217e27e
Author: Sean Rhodes <sean@starlabs.systems>

firmware: coreboot: Add CFR firmware attributes driver

This commit adds a firmware-attributes driver for the coreboot CFR table
entry to describe firmware setup options. It exposes runtime EFI
variable-backed boolean, enum, and integer settings through sysfs.

Link: https://doc.coreboot.org/drivers/cfr.html
Link: https://doc.coreboot.org/drivers/cfr_internal.html

> diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c
> new file mode 100644
> index 0000000000000..1dfb2d29046b6
> --- /dev/null
> +++ b/drivers/firmware/coreboot/coreboot-cfr.c
[ ... ]
> +static int coreboot_cfr_copy_enum_values(struct coreboot_cfr_setting *setting,
> +					 const void *base, size_t len)
> +{
[ ... ]
> +	for (;;) {
> +		entry = coreboot_cfr_next_entry(&iterator);
[ ... ]
> +		value = &setting->values[setting->n_values];
> +		value->label = coreboot_cfr_string_dup(label);
> +		if (!value->label)
> +			return -ENOMEM;
> +		if (!coreboot_cfr_string_is_valid_label(value->label))
> +			return -EINVAL;

[Severity: Medium]
Does this code leak the newly allocated value->label string?

When coreboot_cfr_string_is_valid_label() fails and returns -EINVAL,
setting->n_values has not been incremented yet. If the caller then cleans up
the error by calling coreboot_cfr_free_setting(), the cleanup loop:

static void coreboot_cfr_free_setting(struct coreboot_cfr_setting *setting)
{
	unsigned int i;

	for (i = 0; i < setting->n_values; i++)
		kfree(setting->values[i].label);
	...

will skip freeing the string that was just allocated because setting->n_values
does not include it yet. Should kfree() be called on value->label before
returning -EINVAL here?

> +
> +		value->value = enum_value->value;
> +		setting->n_values++;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785838269.git.sean@starlabs.systems?part=4