[SRU][F][PATCH 1/3] media: cec: abort if the current transmit was canceled

[Magali Lemes]

On 10/01/2025 18:19, Jacob Martin wrote:
> From: Hans Verkuil <hverkuil-cisco at xs4all.nl>
> 
> If a transmit-in-progress was canceled, then, once the transmit
> is done, mark it as aborted and refrain from retrying the transmit.
> 
> To signal this situation the new transmit_in_progress_aborted field is
> set to true.
> 
> The old implementation would just set adap->transmitting to NULL and
> set adap->transmit_in_progress to false, but on the hardware level
> the transmit was still ongoing. However, the framework would think
> the transmit was aborted, and if a new transmit was issued, then
> it could overwrite the HW buffer containing the old transmit with the
> new transmit, leading to garbled data on the CEC bus.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco at xs4all.nl>
> Signed-off-by: Mauro Carvalho Chehab <mchehab at kernel.org>
> (backported from commit 590a8e564c6eff7e77a84e728612f1269e3c0685)
> [jacobmartin: initialize transmit_in_progress_aborted in directly in
> __cec_s_phys_addr as cec_activate_cnt_{inc,dec} from 3813c932ed ("media:
> cec: call enable_adap on s_log_addrs") are not present.]
> CVE-2024-23848
> Signed-off-by: Jacob Martin <jacob.martin at canonical.com>
> ---
>   drivers/media/cec/cec-adap.c | 13 ++++++++++---
>   include/media/cec.h          |  1 +
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> index 94ddaca496c9..cda48e58557c 100644
> --- a/drivers/media/cec/cec-adap.c
> +++ b/drivers/media/cec/cec-adap.c
> @@ -418,7 +418,7 @@ static void cec_flush(struct cec_adapter *adap)
>   		cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
>   	}
>   	if (adap->transmitting)
> -		cec_data_cancel(adap->transmitting, CEC_TX_STATUS_ABORTED);
> +		adap->transmit_in_progress_aborted = true;
>   
>   	/* Cancel the pending timeout work. */
>   	list_for_each_entry_safe(data, n, &adap->wait_queue, list) {
> @@ -569,6 +569,7 @@ int cec_thread_func(void *_adap)
>   		if (data->attempts == 0)
>   			data->attempts = attempts;
>   
> +		adap->transmit_in_progress_aborted = false;
>   		/* Tell the adapter to transmit, cancel on error */
>   		if (adap->ops->adap_transmit(adap, data->attempts,
>   					     signal_free_time, &data->msg))
> @@ -596,6 +597,8 @@ void cec_transmit_done_ts(struct cec_adapter *adap, u8 status,
>   	struct cec_msg *msg;
>   	unsigned int attempts_made = arb_lost_cnt + nack_cnt +
>   				     low_drive_cnt + error_cnt;
> +	bool done = status & (CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_OK);
> +	bool aborted = adap->transmit_in_progress_aborted;
>   
>   	dprintk(2, "%s: status 0x%02x\n", __func__, status);
>   	if (attempts_made < 1)
> @@ -616,6 +619,7 @@ void cec_transmit_done_ts(struct cec_adapter *adap, u8 status,
>   		goto wake_thread;
>   	}
>   	adap->transmit_in_progress = false;
> +	adap->transmit_in_progress_aborted = false;
>   
>   	msg = &data->msg;
>   
> @@ -636,8 +640,7 @@ void cec_transmit_done_ts(struct cec_adapter *adap, u8 status,
>   	 * the hardware didn't signal that it retried itself (by setting
>   	 * CEC_TX_STATUS_MAX_RETRIES), then we will retry ourselves.
>   	 */
> -	if (data->attempts > attempts_made &&
> -	    !(status & (CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_OK))) {
> +	if (!aborted && data->attempts > attempts_made && !done) {
>   		/* Retry this message */
>   		data->attempts -= attempts_made;
>   		if (msg->timeout)
> @@ -652,6 +655,8 @@ void cec_transmit_done_ts(struct cec_adapter *adap, u8 status,
>   		goto wake_thread;
>   	}
>   
> +	if (aborted && !done)
> +		status |= CEC_TX_STATUS_ABORTED;
>   	data->attempts = 0;
>   
>   	/* Always set CEC_TX_STATUS_MAX_RETRIES on error */
> @@ -1571,6 +1576,7 @@ void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>   		if (adap->needs_hpd || list_empty(&adap->devnode.fhs)) {
>   			WARN_ON(adap->ops->adap_enable(adap, false));
>   			adap->transmit_in_progress = false;
> +			adap->transmit_in_progress_aborted = false;
>   			wake_up_interruptible(&adap->kthread_waitq);
>   		}
>   		mutex_unlock(&adap->devnode.lock);
> @@ -1581,6 +1587,7 @@ void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>   	mutex_lock(&adap->devnode.lock);
>   	adap->last_initiator = 0xff;
>   	adap->transmit_in_progress = false;
> +	adap->transmit_in_progress_aborted = false;
>   

Why was the block below
```
	if (adap->transmitting)
		cec_data_cancel(adap->transmitting, CEC_TX_STATUS_ABORTED);
```
from the original upstream commit dropped?


>   	if ((adap->needs_hpd || list_empty(&adap->devnode.fhs)) &&
>   	    adap->ops->adap_enable(adap, true)) {
> diff --git a/include/media/cec.h b/include/media/cec.h
> index 4d59387bc61b..06f88f126828 100644
> --- a/include/media/cec.h
> +++ b/include/media/cec.h
> @@ -187,6 +187,7 @@ struct cec_adapter {
>   	struct list_head wait_queue;
>   	struct cec_data *transmitting;
>   	bool transmit_in_progress;
> +	bool transmit_in_progress_aborted;
>   
>   	struct task_struct *kthread_config;
>   	struct completion config_completion;