From: Cong Zhang cong.zhang@oss.qualcomm.com
[ Upstream commit 0739c2c6a015604a7c01506bea28200a2cc2e08c ]
The vblk->vqs releases during freeze. If resume fails before vblk->vqs is allocated, later freeze/remove may attempt to free vqs again. Set vblk->vqs to NULL after freeing to avoid double free.
Signed-off-by: Cong Zhang cong.zhang@oss.qualcomm.com Acked-by: Jason Wang jasowang@redhat.com Signed-off-by: Jens Axboe axboe@kernel.dk Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
Good. `kfree(NULL)` is safe - it does nothing. This is a well-known C/Linux convention.
Now let me create my final comprehensive analysis:
## COMPLETE ANALYSIS
### 1. COMMIT MESSAGE ANALYSIS
**Subject**: `virtio_blk: NULL out vqs to avoid double free on failed resume`
**Key points from commit message:** - The `vblk->vqs` is released during freeze - If resume fails before `vblk->vqs` is allocated, later freeze/remove may attempt to free vqs again - Solution: Set `vblk->vqs` to NULL after freeing to avoid double free
**Acks/Reviews:** - Acked-by: Jason Wang jasowang@redhat.com (virtio maintainer) - Signed-off-by: Jens Axboe axboe@kernel.dk (block subsystem maintainer)
**Missing tags:** - No `Cc: stable@vger.kernel.org` tag - No `Fixes:` tag explicitly pointing to the bug-introducing commit
### 2. CODE CHANGE ANALYSIS
**Changes made:** Two modifications in `drivers/block/virtio_blk.c`:
#### Change 1: In `init_vq()` error path (lines 1029-1032) **Before:** ```c if (err) kfree(vblk->vqs); return err; ```
**After:** ```c if (err) { kfree(vblk->vqs); /* - Set to NULL to prevent freeing vqs again during freezing. */ vblk->vqs = NULL; } return err; ```
#### Change 2: In `virtblk_freeze_priv()` (lines 1599-1600) **Before:** ```c vdev->config->del_vqs(vdev); kfree(vblk->vqs);
return 0; ```
**After:** ```c vdev->config->del_vqs(vdev); kfree(vblk->vqs); /* - Set to NULL to prevent freeing vqs again after a failed vqs - allocation during resume. Note that kfree() already handles NULL - pointers safely. */ vblk->vqs = NULL;
return 0; ```
### 3. BUG MECHANISM (Root Cause Analysis)
The double-free vulnerability occurs in the following scenario:
**Trigger Sequence:** 1. **virtblk_freeze_priv()** is called (suspend/PM freeze, or reset_prepare via FLR) - Frees `vblk->vqs` at line 1600 - `vblk->vqs` **still points to the freed memory** (dangling pointer)
2. **virtblk_restore_priv()** is called (resume/PM restore, or reset_done) - Calls `init_vq(vblk)` at line 1610
3. **init_vq()** fails (e.g., `kmalloc_array()` fails or `virtio_find_vqs()` fails) - `init_vq()` allocates `vblk->vqs` at line 993 - If allocation succeeds but later `virtio_find_vqs()` fails (line 1016), the error path at line 1030 calls `kfree(vblk->vqs)` - But if allocation at line 993 fails, `vblk->vqs` is never reassigned and still points to the OLD freed memory from step 1 - Error path at line 1030: `kfree(vblk->vqs)` - **FIRST FREE of the OLD pointer**
4. **Second freeze/remove attempt:** - If another freeze cycle or `virtblk_remove()` is called - `kfree(vblk->vqs)` is called again - **SECOND FREE of the same memory = DOUBLE FREE**
**Alternative scenario:** - Even in `init_vq()` success path, if `vqs_info` or `vqs` temp allocation fails before line 997-999, and the error `goto out` is hit, the same dangling pointer issue occurs.
### 4. CLASSIFICATION
- **Type**: Bug fix (memory safety - double-free vulnerability) - **Security relevance**: Potentially exploitable memory corruption bug - **Category**: Does NOT fall into exceptions (device IDs, quirks, DT, build fixes) - **Impact area**: virtio-blk block device driver, PM suspend/resume and transport reset recovery
### 5. SCOPE AND RISK ASSESSMENT
**Lines changed**: ~10 lines (including comments) **Files touched**: 1 file (`drivers/block/virtio_blk.c`) **Complexity**: Very low - simple NULL assignment after kfree
**Subsystem**: virtio-blk - a mature, widely-used block device driver for virtual machines - Used in QEMU/KVM guests - Used in cloud VM instances (AWS, GCP, Azure etc.) - Used in container environments
**Risk assessment**: **VERY LOW** - The fix is trivial: just setting pointer to NULL after free - `kfree(NULL)` is explicitly safe (no-op) - No behavioral change in normal operation - Only affects error recovery paths - Cannot introduce new bugs
### 6. USER IMPACT
**Who is affected:** - Users using virtio-blk devices in virtual machines - Systems that undergo suspend/resume cycles - Systems using PCI Function Level Reset (FLR) on virtio devices - Particularly affects systems with constrained memory where allocation might fail
**Severity if bug hits:** - Double-free can cause kernel panic/crash - Potential memory corruption - Potential security vulnerability (though exploitation would be difficult)
**Trigger conditions:** - Requires PM suspend/resume OR transport reset (FLR) - AND memory allocation failure during resume - Relatively rare in practice but can happen under memory pressure
### 7. STABILITY INDICATORS
**Positive indicators:** - Acked-by: Jason Wang (virtio maintainer) - Signed-off-by: Jens Axboe (block maintainer) - The fix is obviously correct and minimal - Follows the standard kernel pattern of NULLing after free
**Negative indicators:** - No `Cc: stable@vger.kernel.org` tag - No `Fixes:` tag - No `Tested-by:` tag
### 8. DEPENDENCY CHECK
**Dependencies:** - Requires commit `5820a3b089879` ("virtio_blk: Add support for transport error recovery") which introduced `virtblk_freeze_priv()` and `virtblk_restore_priv()` as shared functions - This commit (`5820a3b089879`) was merged in v6.14-rc1
**Earlier bug introduction:** - The original bug was introduced in `b71ba22e7c6c6` ("virtio-blk: Fix memory leak among suspend/resume procedure") in v5.14-rc1 - That commit added `kfree(vblk->vqs)` in freeze without NULLing it
**For stable backport:** - For kernels v6.14+: should apply cleanly - For kernels v5.14 to v6.13: would need different backport (the code structure is different) - The `virtblk_freeze_priv` function only exists starting from v6.14
### 9. CONCLUSION
**This commit SHOULD be backported to stable trees** because:
1. **Fixes a real bug**: Double-free is a serious memory safety bug that can cause crashes and potentially be exploited
2. **Obvious correctness**: Setting pointer to NULL after free is the standard kernel idiom and cannot introduce new bugs
3. **Small and contained**: Only ~10 lines changed, very localized to two specific error handling paths
4. **No new features**: Pure bug fix with no behavioral changes in normal operation
5. **Affects important infrastructure**: virtio-blk is used extensively in cloud/virtualization environments
6. **Low risk**: The fix is trivial and follows standard patterns; `kfree(NULL)` is explicitly safe
**Caveats for backporting:** - The fix in its current form is ONLY applicable to v6.14+ kernels where `virtblk_freeze_priv()` exists - For older stable kernels (v5.14 to v6.13), a different (simpler) backport would be needed that just NULLs `vblk->vqs` in `virtblk_freeze()` directly - The maintainers did not explicitly tag this for stable (`Cc: stable@vger.kernel.org`), which might indicate they didn't consider backporting, or the bug wasn't seen as critical
The fix is small, surgical, obviously correct, and addresses a real memory safety bug. It meets all stable kernel criteria.
**YES**
drivers/block/virtio_blk.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index f061420dfb10c..746795066d7f5 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -1026,8 +1026,13 @@ static int init_vq(struct virtio_blk *vblk) out: kfree(vqs); kfree(vqs_info); - if (err) + if (err) { kfree(vblk->vqs); + /* + * Set to NULL to prevent freeing vqs again during freezing. + */ + vblk->vqs = NULL; + } return err; }
@@ -1598,6 +1603,12 @@ static int virtblk_freeze_priv(struct virtio_device *vdev)
vdev->config->del_vqs(vdev); kfree(vblk->vqs); + /* + * Set to NULL to prevent freeing vqs again after a failed vqs + * allocation during resume. Note that kfree() already handles NULL + * pointers safely. + */ + vblk->vqs = NULL;
return 0; }