Re: [PATCH v2] s390: raw3270: Handle allocation failures

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

Pre-existing issues:
- [High] Dangling pointer and Use-After-Free if `ccw_device_enable_console()` fails in `raw3270_setup_console()`.
- [High] Device reference count leak in `raw3270_create_device()` on error path.
--

--- Patch [1]: [PATCH v2] s390: raw3270: Handle allocation failures ---
commit 0dc799bc2a1ab261a4df0f1061360db8eed525e2
Author: Triet Hoang <[email protected]>

s390: raw3270: Handle allocation failures

Check the return values of kzalloc_obj() and kzalloc() before using the
allocated memory. Free the allocated resources on subsequent initialization
failures to avoid leaking memory.

> diff --git a/drivers/s390/char/raw3270.c b/drivers/s390/char/raw3270.c
> index aa9c4d81225cb..ff397396b440e 100644
> --- a/drivers/s390/char/raw3270.c
> +++ b/drivers/s390/char/raw3270.c
> @@ -814,10 +814,23 @@ struct raw3270 __init *raw3270_setup_console(void)
>  		return ERR_CAST(cdev);
>  
>  	rp = kzalloc_obj(*rp, GFP_KERNEL | GFP_DMA);
> +	if (!rp) {
> +		ccw_device_destroy_console(cdev);
> +		return ERR_PTR(-ENOMEM);
> +	}
>  	ascebc = kzalloc(256, GFP_KERNEL);
> +	if (!ascebc) {
> +		ccw_device_destroy_console(cdev);
> +		kfree(rp);
> +		return ERR_PTR(-ENOMEM);
> +	}
>  	rc = raw3270_setup_device(cdev, rp, ascebc);
> -	if (rc)
> +	if (rc) {
> +		ccw_device_destroy_console(cdev);
> +		kfree(rp->ascebc);
> +		kfree(rp);
>  		return ERR_PTR(rc);
> +	}
>  	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
>  
>  	rc = ccw_device_enable_console(cdev);

[Severity: High]
This isn't a bug introduced by this patch, but if ccw_device_enable_console()
fails just below this context:

drivers/s390/char/raw3270.c:raw3270_setup_console() {
    ...
	rc = ccw_device_enable_console(cdev);
	if (rc) {
		ccw_device_destroy_console(cdev);
		return ERR_PTR(rc);
	}
    ...
}

This error path destroys the cdev but fails to unlink rp from
raw3270_devices or free it.

Because raw3270_setup_device() previously added rp to the global list here:

drivers/s390/char/raw3270.c:raw3270_setup_device() {
    ...
    if (rp->minor == -1 && minor < RAW3270_MAXDEVS + RAW3270_FIRSTMINOR) {
        rp->minor = minor;
        list_add_tail(&rp->list, &raw3270_devices);
    }
    ...
}

Does leaving it linked in raw3270_devices create a dangling pointer where
rp->cdev points to the destroyed console device?

Later, when raw3270_init() iterates over this list, it could dereference
rp->cdev->dev, causing a use-after-free.

I also noticed a pre-existing resource leak in the same file:

drivers/s390/char/raw3270.c:raw3270_create_device() {
    ...
	rc = raw3270_setup_device(cdev, rp, ascebc);
	if (rc) {
		kfree(rp->ascebc);
		kfree(rp);
		rp = ERR_PTR(rc);
	}
	/* Get reference to ccw_device structure. */
	get_device(&cdev->dev);
	return rp;
}

[Severity: High]
This is a pre-existing issue, but does this leak a device reference count when
raw3270_setup_device() fails?

If raw3270_setup_device() fails, the function continues to unconditionally
execute get_device(&cdev->dev) before returning the error pointer.

The caller, raw3270_set_online(), receives the error and returns immediately,
meaning put_device() is never called to balance it.

Can this permanently leak a reference to the ccw_device, preventing the
physical device's structures from being freed even if hot-removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1