Re: [PATCH v4 8/8] usb: typec: ucsi: huawei-gaokun: pass down HPD_IRQ events

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

Pre-existing issues:
- [High] Early return misses EC hardware acknowledgement, potentially causing an interrupt storm.
- [High] Deadlock between EC threaded IRQ handler and UCSI notifier causing inevitable timeout.
- [High] Out-of-bounds array access due to dynamic `num_ports` update surpassing allocated size.
- [Medium] Undefined Behavior due to shift operation with negative exponent.
--

commit 73f2964a00c46ffc80bf958e8a195aee63f9493d
Author: Dmitry Baryshkov <[email protected]>

usb: typec: ucsi: huawei-gaokun: pass down HPD_IRQ events

Pass IRQ_HPD events from the EC to the HPD bridge, letting those
to be delivered to the DisplayPort driver.

> diff --git a/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c b/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
> --- a/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
> +++ b/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
> @@ -299,10 +299,13 @@ static void gaokun_ucsi_handle_altmode(struct gaokun_ucsi_port *port)

[ ... ]

>  	/* UCSI callback .connector_status() have set orientation */
>  	if (port->bridge)
> -		drm_aux_hpd_bridge_notify(&port->bridge->dev,
> -					  port->hpd_state ?
> -					  connector_status_connected :
> -					  connector_status_disconnected);
> +		drm_aux_hpd_bridge_notify_extra(&port->bridge->dev,
> +						port->hpd_state ?
> +						connector_status_connected :
> +						connector_status_disconnected,
> +						port->hpd_irq ?
> +						DRM_CONNECTOR_DP_IRQ_HPD :
> +						DRM_CONNECTOR_NO_EXTRA_STATUS);
>  
>  	gaokun_ec_ucsi_pan_ack(uec->ec, port->idx);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
issue in gaokun_ucsi_handle_altmode() regarding the early return.

If an EC event has a port index that surpasses the UCSI capability connector
bounds, the function warns and returns early.

Because the event has already been consumed by the hardware, does skipping
the gaokun_ec_ucsi_pan_ack() call here cause the EC firmware to infinitely
resend the interrupt or stall subsequent operations?

[Severity: Medium]
This is a pre-existing issue, but looking at the gaokun_ec_ucsi_pan_ack()
call at the end of this function:

drivers/platform/arm64/huawei-gaokun-ec.c:gaokun_ec_ucsi_pan_ack() {
    ...
    u8 data = 1 << port_id;

    if (port_id == GAOKUN_UCSI_NO_PORT_UPDATE)
        data = 0;
    ...
}

When gaokun_ucsi_altmode_notify_ind() explicitly calls this with
GAOKUN_UCSI_NO_PORT_UPDATE (which equals -1), the shift operation
evaluates unconditionally before the safety check.

Does this cause undefined behavior by shifting with a negative exponent?

>  }

[Severity: High]
This is another pre-existing issue, but I noticed a potential deadlock
between the EC threaded IRQ handler and the UCSI notifier.

If the shared gaokun_ec_irq_handler() invokes gaokun_ucsi_notify() through
the blocking notifier, it can call gaokun_ucsi_handle_no_usb_event() which
waits on a completion:

drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c:gaokun_ucsi_handle_no_usb_event() {
    if (!wait_for_completion_timeout(&port->usb_ack, 2 * HZ)) {
        ...
    }
}

Since the single EC event thread is blocked inside the notifier chain waiting
for this completion, does this prevent the system from extracting the
upcoming EC_EVENT_USB to trigger the completion, imposing a guaranteed
timeout delay?

[Severity: High]
This isn't a bug introduced by this patch, but there might be an
out-of-bounds array access in gaokun_ucsi_refresh() when updating port
data.

During initialization, the uec->ports array is allocated based on the
initial number of ports:

drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c:gaokun_ucsi_ports_init() {
    uec->ports = devm_kcalloc(dev, num_ports, sizeof(*uec->ports),
                              GFP_KERNEL);
    ...
}

In gaokun_ucsi_refresh(), when an interrupt occurs, it reads the new limit
and uses it:

drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c:gaokun_ucsi_refresh() {
    uec->num_ports = ureg.num_ports;
    ...
    gaokun_ucsi_port_update(&uec->ports[idx], ureg.port_data);
}

If the EC dynamically reports a larger number of ports than the original
allocation, does this bypass the boundary check and overwrite unallocated
memory?

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic