Messages in this thread | | | Date | Thu, 11 Jan 2024 09:38:23 +0800 | Subject | Re: [PATCH v2] [media] xc4000: Fix atomicity violation in xc4000_get_frequency | From | 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@vger.kernel.org > Reported-by: BassCheck <bass@buaa.edu.cn> > Signed-off-by: Gui-Dong Han <2045gemini@gmail.com> > --- > 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__); >
| |