From: Xinghui Li <[email protected]>
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 <[email protected]>
Signed-off-by: Xinghui Li <[email protected]>
---
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
Hi Xinghui Li,
On 11/01/2023 13:37, [email protected] wrote:
> From: Xinghui Li <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Xinghui Li <[email protected]>
> ---
> 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;
> }
>
在 2023/1/18 18:18，“Hans Verkuil”<[email protected] <mailto:[email protected]>> 写入:
>...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. __
On 19/01/2023 05:49, Xinghui Li wrote:
> 在 2023/1/18 18:18，“Hans Verkuil”<[email protected] <mailto:[email protected]>> 写入:
>
>> ...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