Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The WIND_RING function fails to rollback the nvif_push buffer state, causing aborted commands to be submitted on the next ioctl. - [High] The legacy pushbuf branch calculates the kernel jump-back address using a stale `chan->dma.cur`, causing an infinite loop when submitting multiple user BOs. -- commit 58dcc07cf127c2fddaeed29612e7fbaebbf1c7ae Author: Junrui Luo <[email protected]> drm/nouveau: bounds-check pushbuf offset/length in legacy DMA path This patch adds bounds checking for pushbuf offset and length in the legacy pre-NV25 DMA path. It rejects out-of-bounds pushes before calling nouveau_bo_wr32() to prevent memory corruption. > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c > b/drivers/gpu/drm/nouveau/nouveau_gem.c > index 20dba02d6175b..e9309279ba8dd 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c [ ... ] > @@ -898,6 +898,17 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void > *data, > cmd = chan->push.addr + ((chan->dma.cur + 2) << 2); [Severity: High] This is a pre-existing issue, but does this calculation use a stale chan->dma.cur? In the legacy chipset branch, the loop calculates the jump back into the kernel pushbuffer using chan->dma.cur, but chan->dma.cur is never incremented within the loop. PUSH_JUMP and PUSH_DATA advance chan->chan.push.cur instead. Since chan->dma.cur isn't updated in the loop, won't all user BOs in a multi-push submission be instructed to jump back to the exact same offset in the kernel pushbuffer? Could this cause the GPU to re-execute the jump to the second user BO and enter an infinite command loop? > cmd |= 0x20000000; > if (unlikely(cmd != req->suffix0)) { > + if (unlikely(push[i].offset + > + push[i].length < 8 || > + push[i].offset + > + push[i].length > > + nvbo->bo.base.size)) { > + NV_PRINTK(err, cli, "push %d buffer not > within bo\n", i); > + WIND_RING(chan); [Severity: High] This isn't a bug introduced by this patch, but does WIND_RING() fully rollback the push buffer state? Looking at WIND_RING(): drivers/gpu/drm/nouveau/nouveau_dma.h:WIND_RING() { chan->dma.cur = chan->dma.put; } It only resets chan->dma.cur. However, the subsequent code uses PUSH_JUMP and PUSH_DATA, which write directly to the nvif_push object and increment chan->chan.push.cur without updating chan->dma.cur. If WIND_RING() is called on an error path, but chan->chan.push.cur is not reset, will the next successful ioctl compute the length including these aborted commands? Since nouveau_bo_wr32() wasn't executed to write the jump-back instruction into the user BO for the aborted push, could committing these aborted commands to the GPU cause a hang? > + ret = -EINVAL; > + goto out; > + } > + > if (!nvbo->kmap.virtual) { > ret = ttm_bo_kmap(&nvbo->bo, 0, > > PFN_UP(nvbo->bo.base.size), -- Sashiko AI review ยท https://sashiko.dev/#/patchset/meypr01mb78861402d9e05e9ca6b67859af...@meypr01mb7886.ausprd01.prod.outlook.com?part=1
