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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b1a8593b-b4f3-b943-39db-ed17679e32cb@xs4all.nl>
Date:   Wed, 18 Jan 2023 11:18:21 +0100
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     korantwork@...il.com, mchehab@...nel.org
Cc:     linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        Xinghui Li <korantli@...cent.com>, loydlv <loydlv@...cent.com>
Subject: Re: [PATCH] media:cec:fix double free and uaf issue when cancel data
 during noblocking

Hi Xinghui Li,

On 11/01/2023 13:37, korantwork@...il.com wrote:
> From: Xinghui Li <korantli@...cent.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@...cent.com>
> Signed-off-by: Xinghui Li <korantli@...cent.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;
>  }
>