Re: [PATCH 1/4] dmaengine: idxd: assign all engines to group 0 in IAA defaults

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] After a device FLR (Function Level Reset), group->num_engines and group->num_wqs are not correctly restored, leading to sysfs breakage.
- [High] Double free of `engine`, `wq`, and related structures in initialization error paths and internal cleanup.
- [High] Lockless modifications to engine and wq groups via sysfs expose a TOCTOU NULL pointer dereference in sysfs show functions and cause race conditions on configuration state.
- [High] Unchecked array accesses to `idxd->wqs[0]` and `idxd->groups[0]` when capability registers report 0 for `max_wqs` or `max_groups`.
--

--- Patch [1]: [PATCH 1/4] dmaengine: idxd: assign all engines to group 0 in IAA defaults ---
commit 62dd36d5ea20980d4a54ca9e9b685288c6ec2b65
Author: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Subject: dmaengine: idxd: assign all engines to group 0 in IAA defaults

The IAA device defaults only assigned engine 0 to group 0, leaving engines 1
through max_engines-1 unassigned. This commit assigns all available engines
to group 0 so that the full hardware parallelism is used out of the box
without requiring manual setup.

> diff --git a/drivers/dma/idxd/defaults.c b/drivers/dma/idxd/defaults.c
> index 2bbbcd02a0dac..26ebfa2ca1448 100644
> --- a/drivers/dma/idxd/defaults.c
> +++ b/drivers/dma/idxd/defaults.c
> @@ -8,6 +8,7 @@ int idxd_load_iaa_device_defaults(struct idxd_device *idxd)
>  	struct idxd_engine *engine;
>  	struct idxd_group *group;
>  	struct idxd_wq *wq;
> +	int i;
>  
>  	if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
>  		return 0;

[Severity: High]
This is a pre-existing issue, but can this function blindly access idxd->wqs[0]
and idxd->groups[0] if the hardware capability registers report 0 for max_wqs
or max_groups?

If an emulated or malfunctioning device reports num_wqs == 0, the arrays
might be allocated with 0 bytes, returning the ZERO_SIZE_PTR.

drivers/dma/idxd/defaults.c:idxd_load_iaa_device_defaults() {
    ...
    if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
        return 0;

    wq = idxd->wqs[0];
    ...
}

Could this dereference the ZERO_SIZE_PTR and cause an out-of-bounds page fault?

[ ... ]

> -	engine = idxd->engines[0];
> -
> -	/* set engine group to 0 */
> -	engine->group = idxd->groups[0];
> -	engine->group->num_engines++;
> +	/* assign all engines to group 0 */
> +	for (i = 0; i < idxd->max_engines; i++) {
> +		engine = idxd->engines[i];
> +		engine->group = group;
> +		group->num_engines++;
> +	}
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does a function level reset fail to properly
restore group->num_engines and group->num_wqs?

When a reset occurs, the state is cleared and the counters are zeroed:

drivers/dma/idxd/device.c:idxd_groups_clear_state() {
    for (i = 0; i < idxd->max_groups; i++) {
        group = idxd->groups[i];
        memset(&group->grpcfg, 0, sizeof(group->grpcfg));
        group->num_engines = 0;
        group->num_wqs = 0;
        ...
    }
}

However, when restoring the configuration, the pointers are restored but the
counters are not updated:

drivers/dma/idxd/init.c:idxd_device_config_restore() {
    for (i = 0; i < idxd->max_engines; i++) {
        struct idxd_engine *saved_engine, *engine;

        saved_engine = idxd_saved->saved_engines[i];
        engine = idxd->engines[i];

        engine->group = saved_engine->group;

        kfree(saved_engine);
    }
}

If an administrator subsequently unassigns an engine via sysfs, could
engine->group->num_engines decrement below zero, resulting in -1 and causing
permanent -EINVAL for future configuration?

[Severity: High]
This is a pre-existing issue, but does this cleanup path double-free the
engine structures?

When initialization fails, put_device() is called which triggers the release
callback:

drivers/dma/idxd/sysfs.c:idxd_conf_engine_release() {
    struct idxd_engine *engine = confdev_to_engine(dev);

    kfree(engine);
}

Immediately after this returns, the cleanup function also frees the engine:

drivers/dma/idxd/init.c:idxd_clean_engines() {
    ...
        conf_dev = engine_confdev(engine);
        put_device(conf_dev);
        kfree(engine);
    ...
}

Can this lead to a use-after-free or double-free corruption?

[Severity: High]
This is a pre-existing issue, but is there a time-of-check to time-of-use race
condition when reading engine groups via sysfs?

The sysfs show function iterates through the engines and performs a lockless
check followed by a dereference:

drivers/dma/idxd/sysfs.c:group_engines_show() {
    ...
        if (!engine->group)
            continue;

        if (engine->group->id == group->id)
    ...
}

Concurrently, the store function modifies this state locklessly:

drivers/dma/idxd/sysfs.c:engine_group_id_store() {
    ...
        if (engine->group) {
            engine->group->num_engines--;
            engine->group = NULL;
        }
    ...
}

If a user writes -1 to unassign an engine while another thread is reading,
could engine->group become NULL after the check but before the dereference,
causing a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-iaa-crypto-fixes-zswap-v1-0-65cac23c684d@intel.com?part=1