[PATCH] media:cec:fix double free and uaf issue when cancel data during noblocking

[PATCH] media:cec:fix double free and uaf issue when cancel data during noblocking

[korantwork]

From: Xinghui Li <korantli@tencent.com>

data could be free when it is not completed during transmit if
the opt is nonblocking.In this case,the regular free could lead
to double-free.So, add the return value '-EPERM' to mark the
above case.

Reported-by: loydlv <loydlv@tencent.com>
Signed-off-by: Xinghui Li <korantli@tencent.com>
---
 drivers/media/cec/core/cec-adap.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
index 4f5ab3cae8a7..c2ba8d1173c1 100644
--- a/drivers/media/cec/core/cec-adap.c
+++ b/drivers/media/cec/core/cec-adap.c
@@ -311,7 +311,7 @@ static void cec_post_state_event(struct cec_adapter *adap)
  *
  * This function is called with adap->lock held.
  */
-static void cec_data_completed(struct cec_data *data)
+static int cec_data_completed(struct cec_data *data)
 {
 	/*
 	 * Delete this transmit from the filehandle's xfer_list since
@@ -339,7 +339,9 @@ static void cec_data_completed(struct cec_data *data)
 		if (data->fh)
 			cec_queue_msg_fh(data->fh, &data->msg);
 		kfree(data);
+		return -EPERM;
 	}
+	return 0;
 }
 
 /*
@@ -349,7 +351,7 @@ static void cec_data_completed(struct cec_data *data)
  *
  * This function is called with adap->lock held.
  */
-static void cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status)
+static int cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status)
 {
 	struct cec_adapter *adap = data->adap;
 
@@ -388,7 +390,7 @@ static void cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status)
 		/* Allow drivers to process the message first */
 		call_op(adap, received, &data->msg);
 
-	cec_data_completed(data);
+	return cec_data_completed(data);
 }
 
 /*
@@ -744,6 +746,7 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 {
 	struct cec_data *data;
 	bool is_raw = msg_is_raw(msg);
+	int ret = 0;
 
 	if (adap->devnode.unregistered)
 		return -ENODEV;
@@ -916,18 +919,20 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	/* Cancel the transmit if it was interrupted */
 	if (!data->completed) {
 		if (data->msg.tx_status & CEC_TX_STATUS_OK)
-			cec_data_cancel(data, CEC_TX_STATUS_OK, CEC_RX_STATUS_ABORTED);
+			ret = cec_data_cancel(data, CEC_TX_STATUS_OK, CEC_RX_STATUS_ABORTED);
 		else
-			cec_data_cancel(data, CEC_TX_STATUS_ABORTED, 0);
+			ret = cec_data_cancel(data, CEC_TX_STATUS_ABORTED, 0);
 	}
 
 	/* The transmit completed (possibly with an error) */
-	*msg = data->msg;
-	if (WARN_ON(!list_empty(&data->list)))
-		list_del(&data->list);
-	if (WARN_ON(!list_empty(&data->xfer_list)))
-		list_del(&data->xfer_list);
-	kfree(data);
+	if (!ret) {
+		*msg = data->msg;
+		if (WARN_ON(!list_empty(&data->list)))
+			list_del(&data->list);
+		if (WARN_ON(!list_empty(&data->xfer_list)))
+			list_del(&data->xfer_list);
+		kfree(data);
+	}
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH] media:cec:fix double free and uaf issue when cancel data during noblocking

[Hans Verkuil]

Hi Xinghui Li,

On 11/01/2023 13:37, korantwork@gmail.com wrote:
> From: Xinghui Li <korantli@tencent.com>
> 
> data could be free when it is not completed during transmit if
> the opt is nonblocking.In this case,the regular free could lead
> to double-free.So, add the return value '-EPERM' to mark the
> above case.
> 
> Reported-by: loydlv <loydlv@tencent.com>
> Signed-off-by: Xinghui Li <korantli@tencent.com>
> ---
>  drivers/media/cec/core/cec-adap.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index 4f5ab3cae8a7..c2ba8d1173c1 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -311,7 +311,7 @@ static void cec_post_state_event(struct cec_adapter *adap)
>   *
>   * This function is called with adap->lock held.
>   */
> -static void cec_data_completed(struct cec_data *data)
> +static int cec_data_completed(struct cec_data *data)
>  {
>  	/*
>  	 * Delete this transmit from the filehandle's xfer_list since
> @@ -339,7 +339,9 @@ static void cec_data_completed(struct cec_data *data)
>  		if (data->fh)
>  			cec_queue_msg_fh(data->fh, &data->msg);
>  		kfree(data);
> +		return -EPERM;

This free is called if data->blocking is false...

>  	}
> +	return 0;
>  }
>  
>  /*
> @@ -349,7 +351,7 @@ static void cec_data_completed(struct cec_data *data)
>   *
>   * This function is called with adap->lock held.
>   */
> -static void cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status)
> +static int cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status)
>  {
>  	struct cec_adapter *adap = data->adap;
>  
> @@ -388,7 +390,7 @@ static void cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status)
>  		/* Allow drivers to process the message first */
>  		call_op(adap, received, &data->msg);
>  
> -	cec_data_completed(data);
> +	return cec_data_completed(data);
>  }
>  
>  /*
> @@ -744,6 +746,7 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
>  {
>  	struct cec_data *data;
>  	bool is_raw = msg_is_raw(msg);
> +	int ret = 0;
>  
>  	if (adap->devnode.unregistered)
>  		return -ENODEV;
> @@ -916,18 +919,20 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
>  	/* Cancel the transmit if it was interrupted */
>  	if (!data->completed) {
>  		if (data->msg.tx_status & CEC_TX_STATUS_OK)
> -			cec_data_cancel(data, CEC_TX_STATUS_OK, CEC_RX_STATUS_ABORTED);
> +			ret = cec_data_cancel(data, CEC_TX_STATUS_OK, CEC_RX_STATUS_ABORTED);
>  		else
> -			cec_data_cancel(data, CEC_TX_STATUS_ABORTED, 0);
> +			ret = cec_data_cancel(data, CEC_TX_STATUS_ABORTED, 0);
>  	}
>  
>  	/* The transmit completed (possibly with an error) */
> -	*msg = data->msg;
> -	if (WARN_ON(!list_empty(&data->list)))
> -		list_del(&data->list);
> -	if (WARN_ON(!list_empty(&data->xfer_list)))
> -		list_del(&data->xfer_list);
> -	kfree(data);
> +	if (!ret) {
> +		*msg = data->msg;
> +		if (WARN_ON(!list_empty(&data->list)))
> +			list_del(&data->list);
> +		if (WARN_ON(!list_empty(&data->xfer_list)))
> +			list_del(&data->xfer_list);
> +		kfree(data);

...while this free is called if data->blocking is true. (see the 'if (!block) return 0;'
further up).

So I have my doubts if this patch actually addresses the correct issue.

Do you have an actual debug trace of the UAF? Or even better, code to reproduce
this issue.

Regards,

	Hans

> +	}
>  	return 0;
>  }
>  

Re: [PATCH] media:cec:fix double free and uaf issue when cancel data during noblocking

[Xinghui Li]

在 2023/1/18 18:18，“Hans Verkuil”<hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>> 写入:

>...while this free is called if data->blocking is true. (see the 'if (!block) return 0;'
>further up).
Do you mean this code?

	/* All done if we don't need to block waiting for completion */
	if (!block)
		return 0;
I notice this part code. But I'm not sure if 'block' will be modified in other sync operations. 
So I sent this patch for community to review.

>So I have my doubts if this patch actually addresses the correct issue.
>Do you have an actual debug trace of the UAF? Or even better, code to reproduce
>this issue.

And we found this issue by the code scanning tool developed by loydlv and filtered from 200 issue by human.
So it could be the none-issue. If so, I hope I didn't waste too much of your time. __

Re: [PATCH] media:cec:fix double free and uaf issue when cancel data during noblocking

[Hans Verkuil]

On 19/01/2023 05:49, Xinghui Li wrote:
> 在 2023/1/18 18:18，“Hans Verkuil”<hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>> 写入:
> 
>> ...while this free is called if data->blocking is true. (see the 'if (!block) return 0;'
>> further up).
> Do you mean this code?
> 
> 	/* All done if we don't need to block waiting for completion */
> 	if (!block)
> 		return 0;

Yes.

> I notice this part code. But I'm not sure if 'block' will be modified in other sync operations. 
> So I sent this patch for community to review.

It's not modified anywhere else.

> 
>> So I have my doubts if this patch actually addresses the correct issue.
>> Do you have an actual debug trace of the UAF? Or even better, code to reproduce
>> this issue.
> 
> And we found this issue by the code scanning tool developed by loydlv and filtered from 200 issue by human.
> So it could be the none-issue. If so, I hope I didn't waste too much of your time.

I'll reject this patch since I believe this to be a false report. For future reference:
if a patch is based on code scanning tools then it's good to mention that in the commit
log. I wasn't aware that 'loydlv' is such a tool.

Regards,

	Hans