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

[Jacob Martin]

On 1/13/25 9:26 AM, Magali Lemes wrote:
> 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?
> 

It shouldn't have been! Thanks, I'll send a v2.

> 
>>       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;
>