Re: [PATCH v2] [media] xc4000: Fix atomicity violation in xc4000_get_frequency

[Gui-Dong Han]

Dear All:

I hope this email finds you well. I hope you haven't missed my previous 
email, as I understand that everyone has a busy schedule. I just wanted 
to follow up on my previous message sent.
I understand that you may be occupied with other tasks or priorities. 
However, I would greatly appreciate it if you could spare a few moments 
to check the patch in my previous email. Your prompt response would be 
highly valuable to me.
Thank you for your attention to this matter, and I look forward to 
hearing from you soon.

Thanks,
Han

On 22/12/2023 下午1:50, Gui-Dong Han wrote:
> In xc4000_get_frequency():
> 	*freq = priv->freq_hz + priv->freq_offset;
> The code accesses priv->freq_hz and priv->freq_offset without holding any
> lock.
>
> In xc4000_set_params():
> 	// Code that updates priv->freq_hz and priv->freq_offset
> 	...
>
> xc4000_get_frequency() and xc4000_set_params() may execute concurrently,
> risking inconsistent reads of priv->freq_hz and priv->freq_offset. Since
> these related data may update during reading, it can result in incorrect
> frequency calculation, leading to atomicity violations.
>
> This possible bug is found by an experimental static analysis tool
> developed by our team, BassCheck[1]. This tool analyzes the locking APIs
> to extract function pairs that can be concurrently executed, and then
> analyzes the instructions in the paired functions to identify possible
> concurrency bugs including data races and atomicity violations. The above
> possible bug is reported when our tool analyzes the source code of
> Linux 6.2.
>
> To address this issue, it is proposed to add a mutex lock pair in
> xc4000_get_frequency() to ensure atomicity. With this patch applied, our
> tool no longer reports the possible bug, with the kernel configuration
> allyesconfig for x86_64. Due to the lack of associated hardware, we cannot
> test the patch in runtime testing, and just verify it according to the
> code logic.
>
> [1] https://sites.google.com/view/basscheck/
>
> Fixes: 4c07e32884ab6 ("[media] xc4000: Fix get_frequency()")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: BassCheck <bass@xxxxxxxxxxx>
> Signed-off-by: Gui-Dong Han <2045gemini@xxxxxxxxx>
> ---
> v2:
> * In this patch v2, we've added some information of the static analysis
> tool used, as per the researcher guidelines. Also, we've added a cc in the
> signed-off-by area, according to the stable-kernel-rules.
>    Thank Greg KH for helpful advice.
> ---
>   drivers/media/tuners/xc4000.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/tuners/xc4000.c b/drivers/media/tuners/xc4000.c
> index 57ded9ff3f04..29bc63021c5a 100644
> --- a/drivers/media/tuners/xc4000.c
> +++ b/drivers/media/tuners/xc4000.c
> @@ -1515,10 +1515,10 @@ static int xc4000_get_frequency(struct dvb_frontend *fe, u32 *freq)
>   {
>   	struct xc4000_priv *priv = fe->tuner_priv;
>   
> +	mutex_lock(&priv->lock);
>   	*freq = priv->freq_hz + priv->freq_offset;
>   
>   	if (debug) {
> -		mutex_lock(&priv->lock);
>   		if ((priv->cur_fw.type
>   		     & (BASE | FM | DTV6 | DTV7 | DTV78 | DTV8)) == BASE) {
>   			u16	snr = 0;
> @@ -1529,8 +1529,8 @@ static int xc4000_get_frequency(struct dvb_frontend *fe, u32 *freq)
>   				return 0;
>   			}
>   		}
> -		mutex_unlock(&priv->lock);
>   	}
> +	mutex_unlock(&priv->lock);
>   
>   	dprintk(1, "%s()\n", __func__);
>