| Message ID | 20241030064824.6122-1-chenqiuji666@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] drivers:media:radio: Fix atomicity violation in fmc_send_cmd() | expand |
Hi Qiu-ji Chen, On 30/10/2024 07:48, Qiu-ji Chen wrote: > Atomicity violation occurs when the fmc_send_cmd() function is executed > simultaneously with the modification of the fmdev->resp_skb value. > Consider a scenario where, after passing the validity check within the > function, a non-null fmdev->resp_skb variable is assigned a null value. > This results in an invalid fmdev->resp_skb variable passing the validity > check. As seen in the later part of the function, skb = fmdev->resp_skb; > when the invalid fmdev->resp_skb passes the check, a null pointer > dereference error may occur at line 478, evt_hdr = (void *)skb->data; > > To address this issue, it is recommended to include the validity check of > fmdev->resp_skb within the locked section of the function. This > modification ensures that the value of fmdev->resp_skb does not change > during the validation process, thereby maintaining its validity. > > This possible bug is found by an experimental static analysis tool > developed by our team. 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. FYI, since this driver will be removed soon, I'm not taking this patch. See this patch for the driver removal: https://patchwork.linuxtv.org/project/linux-media/patch/20241028083030.26351-1-lukas.bulwahn@redhat.com/ Regards, Hans > > Fixes: e8454ff7b9a4 ("[media] drivers:media:radio: wl128x: FM Driver Common sources") > Cc: stable@vger.kernel.org > Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com> > --- > drivers/media/radio/wl128x/fmdrv_common.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c > index 3d36f323a8f8..4d032436691c 100644 > --- a/drivers/media/radio/wl128x/fmdrv_common.c > +++ b/drivers/media/radio/wl128x/fmdrv_common.c > @@ -466,11 +466,12 @@ int fmc_send_cmd(struct fmdev *fmdev, u8 fm_op, u16 type, void *payload, > jiffies_to_msecs(FM_DRV_TX_TIMEOUT) / 1000); > return -ETIMEDOUT; > } > + spin_lock_irqsave(&fmdev->resp_skb_lock, flags); > if (!fmdev->resp_skb) { > + spin_unlock_irqrestore(&fmdev->resp_skb_lock, flags); > fmerr("Response SKB is missing\n"); > return -EFAULT; > } > - spin_lock_irqsave(&fmdev->resp_skb_lock, flags); > skb = fmdev->resp_skb; > fmdev->resp_skb = NULL; > spin_unlock_irqrestore(&fmdev->resp_skb_lock, flags);
diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c index 3d36f323a8f8..4d032436691c 100644 --- a/drivers/media/radio/wl128x/fmdrv_common.c +++ b/drivers/media/radio/wl128x/fmdrv_common.c @@ -466,11 +466,12 @@ int fmc_send_cmd(struct fmdev *fmdev, u8 fm_op, u16 type, void *payload, jiffies_to_msecs(FM_DRV_TX_TIMEOUT) / 1000); return -ETIMEDOUT; } + spin_lock_irqsave(&fmdev->resp_skb_lock, flags); if (!fmdev->resp_skb) { + spin_unlock_irqrestore(&fmdev->resp_skb_lock, flags); fmerr("Response SKB is missing\n"); return -EFAULT; } - spin_lock_irqsave(&fmdev->resp_skb_lock, flags); skb = fmdev->resp_skb; fmdev->resp_skb = NULL; spin_unlock_irqrestore(&fmdev->resp_skb_lock, flags);
Atomicity violation occurs when the fmc_send_cmd() function is executed simultaneously with the modification of the fmdev->resp_skb value. Consider a scenario where, after passing the validity check within the function, a non-null fmdev->resp_skb variable is assigned a null value. This results in an invalid fmdev->resp_skb variable passing the validity check. As seen in the later part of the function, skb = fmdev->resp_skb; when the invalid fmdev->resp_skb passes the check, a null pointer dereference error may occur at line 478, evt_hdr = (void *)skb->data; To address this issue, it is recommended to include the validity check of fmdev->resp_skb within the locked section of the function. This modification ensures that the value of fmdev->resp_skb does not change during the validation process, thereby maintaining its validity. This possible bug is found by an experimental static analysis tool developed by our team. 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. Fixes: e8454ff7b9a4 ("[media] drivers:media:radio: wl128x: FM Driver Common sources") Cc: stable@vger.kernel.org Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com> --- drivers/media/radio/wl128x/fmdrv_common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)