* [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
@ 2021-05-11 16:35 Karol Herbst
2021-05-13 15:25 ` Jeremy Cline
2021-05-17 8:56 ` Thierry Reding
0 siblings, 2 replies; 9+ messages in thread
From: Karol Herbst @ 2021-05-11 16:35 UTC (permalink / raw)
To: nouveau; +Cc: Thierry Reding
If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
do it through nouveau_bo_ref.
==================================================================
BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
Use-after-free write at 0x000000004dc4663c (in kfence-#44):
ttm_bo_put+0x11/0x40 [ttm]
nouveau_gem_new+0xc1/0xf0 [nouveau]
nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
drm_ioctl_kernel+0xb2/0x100 [drm]
drm_ioctl+0x215/0x390 [drm]
nouveau_drm_ioctl+0x55/0xa0 [nouveau]
__x64_sys_ioctl+0x83/0xb0
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xae
kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
nouveau_bo_alloc+0x63/0x4c0 [nouveau]
nouveau_gem_new+0x38/0xf0 [nouveau]
nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
drm_ioctl_kernel+0xb2/0x100 [drm]
drm_ioctl+0x215/0x390 [drm]
nouveau_drm_ioctl+0x55/0xa0 [nouveau]
__x64_sys_ioctl+0x83/0xb0
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xae
freed by task 2657:
ttm_bo_release+0x1cc/0x300 [ttm]
ttm_bo_init_reserved+0x2ec/0x300 [ttm]
ttm_bo_init+0x5e/0xd0 [ttm]
nouveau_bo_init+0xaf/0xc0 [nouveau]
nouveau_gem_new+0x7f/0xf0 [nouveau]
nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
drm_ioctl_kernel+0xb2/0x100 [drm]
drm_ioctl+0x215/0x390 [drm]
nouveau_drm_ioctl+0x55/0xa0 [nouveau]
__x64_sys_ioctl+0x83/0xb0
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xae
Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Karol Herbst <kherbst@redhat.com>
---
drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index c88cbb85f101..1165ff990fb5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
if (ret) {
- nouveau_bo_ref(NULL, &nvbo);
return ret;
}
--
2.31.1
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-11 16:35 [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new Karol Herbst
@ 2021-05-13 15:25 ` Jeremy Cline
2021-05-13 17:37 ` Karol Herbst
2021-05-17 8:56 ` Thierry Reding
1 sibling, 1 reply; 9+ messages in thread
From: Jeremy Cline @ 2021-05-13 15:25 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau, Thierry Reding
On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> do it through nouveau_bo_ref.
>
> ==================================================================
> BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
>
> Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> ttm_bo_put+0x11/0x40 [ttm]
> nouveau_gem_new+0xc1/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> nouveau_gem_new+0x38/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> freed by task 2657:
> ttm_bo_release+0x1cc/0x300 [ttm]
> ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> ttm_bo_init+0x5e/0xd0 [ttm]
> nouveau_bo_init+0xaf/0xc0 [nouveau]
> nouveau_gem_new+0x7f/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Karol Herbst <kherbst@redhat.com>
> ---
> drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index c88cbb85f101..1165ff990fb5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
>
> ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> if (ret) {
> - nouveau_bo_ref(NULL, &nvbo);
> return ret;
> }
>
I submitted a patch for this in the beginning of December, it got
reviewed and it's languished ever since[0]. It's frustrating that these
issues have to get fixed multiple times by multiple developers because
bug fixes (with reviews!) aren't being accepted.
[0] https://lists.freedesktop.org/archives/nouveau/2020-December/037571.html
Anyway, for whatever it's worth:
Reviewed-by: Jeremy Cline <jcline@redhat.com>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-13 15:25 ` Jeremy Cline
@ 2021-05-13 17:37 ` Karol Herbst
2021-05-13 18:22 ` Jeremy Cline
0 siblings, 1 reply; 9+ messages in thread
From: Karol Herbst @ 2021-05-13 17:37 UTC (permalink / raw)
To: Jeremy Cline; +Cc: nouveau, Thierry Reding
On Thu, May 13, 2021 at 5:25 PM Jeremy Cline <jcline@redhat.com> wrote:
>
> On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > do it through nouveau_bo_ref.
> >
> > ==================================================================
> > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> >
> > Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> > ttm_bo_put+0x11/0x40 [ttm]
> > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > nouveau_gem_new+0x38/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > freed by task 2657:
> > ttm_bo_release+0x1cc/0x300 [ttm]
> > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > ttm_bo_init+0x5e/0xd0 [ttm]
> > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Karol Herbst <kherbst@redhat.com>
> > ---
> > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..1165ff990fb5 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, &nvbo);
> > return ret;
> > }
> >
>
> I submitted a patch for this in the beginning of December, it got
> reviewed and it's languished ever since[0]. It's frustrating that these
> issues have to get fixed multiple times by multiple developers because
> bug fixes (with reviews!) aren't being accepted.
>
> [0] https://lists.freedesktop.org/archives/nouveau/2020-December/037571.html
>
> Anyway, for whatever it's worth:
>
> Reviewed-by: Jeremy Cline <jcline@redhat.com>
>
oh indeed... as the patches are equivalent I'd say we should merge
yours and add my
Reviewed-by: Karol Herbst <kherbst@redhat.com>
let's see if we can get it in this time *sigh*
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-13 17:37 ` Karol Herbst
@ 2021-05-13 18:22 ` Jeremy Cline
0 siblings, 0 replies; 9+ messages in thread
From: Jeremy Cline @ 2021-05-13 18:22 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau, Thierry Reding
On Thu, May 13, 2021 at 07:37:40PM +0200, Karol Herbst wrote:
> On Thu, May 13, 2021 at 5:25 PM Jeremy Cline <jcline@redhat.com> wrote:
> >
> > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > > do it through nouveau_bo_ref.
> > >
> > > ==================================================================
> > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > >
> > > Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> > > ttm_bo_put+0x11/0x40 [ttm]
> > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > freed by task 2657:
> > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > > Cc: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Karol Herbst <kherbst@redhat.com>
> > > ---
> > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..1165ff990fb5 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, &nvbo);
> > > return ret;
> > > }
> > >
> >
> > I submitted a patch for this in the beginning of December, it got
> > reviewed and it's languished ever since[0]. It's frustrating that these
> > issues have to get fixed multiple times by multiple developers because
> > bug fixes (with reviews!) aren't being accepted.
> >
> > [0] https://lists.freedesktop.org/archives/nouveau/2020-December/037571.html
> >
> > Anyway, for whatever it's worth:
> >
> > Reviewed-by: Jeremy Cline <jcline@redhat.com>
> >
>
> oh indeed... as the patches are equivalent I'd say we should merge
> yours and add my
>
> Reviewed-by: Karol Herbst <kherbst@redhat.com>
>
> let's see if we can get it in this time *sigh*
>
To be clear I don't care about the credit, accepting this patch is fine,
but I wanted to highlight how multiple developers are doing the same
work over and over because it seems nigh on impossible to get nouveau
kernel patches accepted.
- Jeremy
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-11 16:35 [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new Karol Herbst
2021-05-13 15:25 ` Jeremy Cline
@ 2021-05-17 8:56 ` Thierry Reding
2021-05-17 9:19 ` Thierry Reding
1 sibling, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2021-05-17 8:56 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau
[-- Attachment #1.1: Type: text/plain, Size: 3666 bytes --]
On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> do it through nouveau_bo_ref.
>
> ==================================================================
> BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
>
> Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> ttm_bo_put+0x11/0x40 [ttm]
> nouveau_gem_new+0xc1/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> nouveau_gem_new+0x38/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> freed by task 2657:
> ttm_bo_release+0x1cc/0x300 [ttm]
> ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> ttm_bo_init+0x5e/0xd0 [ttm]
> nouveau_bo_init+0xaf/0xc0 [nouveau]
> nouveau_gem_new+0x7f/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Karol Herbst <kherbst@redhat.com>
> ---
> drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index c88cbb85f101..1165ff990fb5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
>
> ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> if (ret) {
> - nouveau_bo_ref(NULL, &nvbo);
> return ret;
> }
Looking at the surrounding code, I wonder if I just managed to jumble
the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
drm_gem_object_init() fails, I don't think it's necessary (though it
also doesn't look harmful) to call drm_gem_object_release().
However, if nouveau_bo_init() fails, then I think we'd still need to
call drm_gem_object_release(), to make sure to undo the effects of
drm_gem_object_init().
So I wonder if we need something like this instead:
--- >8 ---
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index c88cbb85f101..9b6055116f30 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
* to the caller, instead of a normal nouveau_bo ttm reference. */
ret = drm_gem_object_init(drm->dev, &nvbo->bo.base, size);
if (ret) {
- drm_gem_object_release(&nvbo->bo.base);
kfree(nvbo);
return ret;
}
ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
if (ret) {
- nouveau_bo_ref(NULL, &nvbo);
+ drm_gem_object_release(&nvbo->bo.base);
return ret;
}
--- >8 ---
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-17 8:56 ` Thierry Reding
@ 2021-05-17 9:19 ` Thierry Reding
2021-05-17 12:28 ` Karol Herbst
2021-05-17 13:32 ` Jeremy Cline
0 siblings, 2 replies; 9+ messages in thread
From: Thierry Reding @ 2021-05-17 9:19 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau
[-- Attachment #1.1: Type: text/plain, Size: 3969 bytes --]
On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > do it through nouveau_bo_ref.
> >
> > ==================================================================
> > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> >
> > Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> > ttm_bo_put+0x11/0x40 [ttm]
> > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > nouveau_gem_new+0x38/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > freed by task 2657:
> > ttm_bo_release+0x1cc/0x300 [ttm]
> > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > ttm_bo_init+0x5e/0xd0 [ttm]
> > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Karol Herbst <kherbst@redhat.com>
> > ---
> > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..1165ff990fb5 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, &nvbo);
> > return ret;
> > }
>
> Looking at the surrounding code, I wonder if I just managed to jumble
> the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> drm_gem_object_init() fails, I don't think it's necessary (though it
> also doesn't look harmful) to call drm_gem_object_release().
>
> However, if nouveau_bo_init() fails, then I think we'd still need to
> call drm_gem_object_release(), to make sure to undo the effects of
> drm_gem_object_init().
>
> So I wonder if we need something like this instead:
>
> --- >8 ---
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index c88cbb85f101..9b6055116f30 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> * to the caller, instead of a normal nouveau_bo ttm reference. */
> ret = drm_gem_object_init(drm->dev, &nvbo->bo.base, size);
> if (ret) {
> - drm_gem_object_release(&nvbo->bo.base);
> kfree(nvbo);
> return ret;
> }
>
> ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> if (ret) {
> - nouveau_bo_ref(NULL, &nvbo);
> + drm_gem_object_release(&nvbo->bo.base);
> return ret;
> }
>
> --- >8 ---
>
> Thierry
Adding Jeremy for visibility.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-17 9:19 ` Thierry Reding
@ 2021-05-17 12:28 ` Karol Herbst
2021-05-17 13:32 ` Jeremy Cline
1 sibling, 0 replies; 9+ messages in thread
From: Karol Herbst @ 2021-05-17 12:28 UTC (permalink / raw)
To: Thierry Reding; +Cc: nouveau
On Mon, May 17, 2021 at 11:17 AM Thierry Reding <treding@nvidia.com> wrote:
>
> On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > > do it through nouveau_bo_ref.
> > >
> > > ==================================================================
> > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > >
> > > Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> > > ttm_bo_put+0x11/0x40 [ttm]
> > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > freed by task 2657:
> > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > > Cc: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Karol Herbst <kherbst@redhat.com>
> > > ---
> > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..1165ff990fb5 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, &nvbo);
> > > return ret;
> > > }
> >
> > Looking at the surrounding code, I wonder if I just managed to jumble
> > the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> > drm_gem_object_init() fails, I don't think it's necessary (though it
> > also doesn't look harmful) to call drm_gem_object_release().
> >
> > However, if nouveau_bo_init() fails, then I think we'd still need to
> > call drm_gem_object_release(), to make sure to undo the effects of
> > drm_gem_object_init().
> >
> > So I wonder if we need something like this instead:
> >
> > --- >8 ---
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..9b6055116f30 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > * to the caller, instead of a normal nouveau_bo ttm reference. */
> > ret = drm_gem_object_init(drm->dev, &nvbo->bo.base, size);
> > if (ret) {
> > - drm_gem_object_release(&nvbo->bo.base);
> > kfree(nvbo);
> > return ret;
> > }
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, &nvbo);
> > + drm_gem_object_release(&nvbo->bo.base);
> > return ret;
> > }
> >
I was looking at this already and fixed the above part in
925681454d7b557d404b5d28ef4469fac1b2e105, but yeah.. maybe calling
drm_gem_object_release up there is indeed not needed. Would have to
take a deeper look as well.
> > --- >8 ---
> >
> > Thierry
>
> Adding Jeremy for visibility.
>
> Thierry
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-17 9:19 ` Thierry Reding
2021-05-17 12:28 ` Karol Herbst
@ 2021-05-17 13:32 ` Jeremy Cline
2021-05-17 15:36 ` Thierry Reding
1 sibling, 1 reply; 9+ messages in thread
From: Jeremy Cline @ 2021-05-17 13:32 UTC (permalink / raw)
To: Thierry Reding; +Cc: nouveau
On Mon, May 17, 2021 at 11:19:02AM +0200, Thierry Reding wrote:
> On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > > do it through nouveau_bo_ref.
> > >
> > > ==================================================================
> > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > >
> > > Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> > > ttm_bo_put+0x11/0x40 [ttm]
> > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > freed by task 2657:
> > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > > Cc: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Karol Herbst <kherbst@redhat.com>
> > > ---
> > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..1165ff990fb5 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, &nvbo);
> > > return ret;
> > > }
> >
> > Looking at the surrounding code, I wonder if I just managed to jumble
> > the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> > drm_gem_object_init() fails, I don't think it's necessary (though it
> > also doesn't look harmful) to call drm_gem_object_release().
> >
> > However, if nouveau_bo_init() fails, then I think we'd still need to
> > call drm_gem_object_release(), to make sure to undo the effects of
> > drm_gem_object_init().
> >
> > So I wonder if we need something like this instead:
> >
> > --- >8 ---
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..9b6055116f30 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > * to the caller, instead of a normal nouveau_bo ttm reference. */
> > ret = drm_gem_object_init(drm->dev, &nvbo->bo.base, size);
> > if (ret) {
> > - drm_gem_object_release(&nvbo->bo.base);
> > kfree(nvbo);
> > return ret;
> > }
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, &nvbo);
> > + drm_gem_object_release(&nvbo->bo.base);
> > return ret;
> > }
> >
> > --- >8 ---
> >
> > Thierry
>
> Adding Jeremy for visibility.
>
Admittedly I only skimmed the code so I'm not extremely confident in my
analysis, but isn't that handled by the nouveau_bo_del_ttm() callback
which should get called after the last reference is dropped with
nouveau_bo_ref?
- Jeremy
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
2021-05-17 13:32 ` Jeremy Cline
@ 2021-05-17 15:36 ` Thierry Reding
0 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2021-05-17 15:36 UTC (permalink / raw)
To: Jeremy Cline; +Cc: nouveau
[-- Attachment #1.1: Type: text/plain, Size: 5056 bytes --]
On Mon, May 17, 2021 at 09:32:44AM -0400, Jeremy Cline wrote:
> On Mon, May 17, 2021 at 11:19:02AM +0200, Thierry Reding wrote:
> > On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> > > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > > > do it through nouveau_bo_ref.
> > > >
> > > > ==================================================================
> > > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > > >
> > > > Use-after-free write at 0x000000004dc4663c (in kfence-#44):
> > > > ttm_bo_put+0x11/0x40 [ttm]
> > > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > > drm_ioctl+0x215/0x390 [drm]
> > > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > > __x64_sys_ioctl+0x83/0xb0
> > > > do_syscall_64+0x33/0x40
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > kfence-#44 [0x00000000c0593b31-0x000000002e74122b, size=792, cache=kmalloc-1k] allocated by task 2657:
> > > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > > drm_ioctl+0x215/0x390 [drm]
> > > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > > __x64_sys_ioctl+0x83/0xb0
> > > > do_syscall_64+0x33/0x40
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > freed by task 2657:
> > > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > > drm_ioctl+0x215/0x390 [drm]
> > > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > > __x64_sys_ioctl+0x83/0xb0
> > > > do_syscall_64+0x33/0x40
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > > > Cc: Thierry Reding <treding@nvidia.com>
> > > > Signed-off-by: Karol Herbst <kherbst@redhat.com>
> > > > ---
> > > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > > 1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > index c88cbb85f101..1165ff990fb5 100644
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > > >
> > > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > > if (ret) {
> > > > - nouveau_bo_ref(NULL, &nvbo);
> > > > return ret;
> > > > }
> > >
> > > Looking at the surrounding code, I wonder if I just managed to jumble
> > > the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> > > drm_gem_object_init() fails, I don't think it's necessary (though it
> > > also doesn't look harmful) to call drm_gem_object_release().
> > >
> > > However, if nouveau_bo_init() fails, then I think we'd still need to
> > > call drm_gem_object_release(), to make sure to undo the effects of
> > > drm_gem_object_init().
> > >
> > > So I wonder if we need something like this instead:
> > >
> > > --- >8 ---
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..9b6055116f30 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t domain,
> > > * to the caller, instead of a normal nouveau_bo ttm reference. */
> > > ret = drm_gem_object_init(drm->dev, &nvbo->bo.base, size);
> > > if (ret) {
> > > - drm_gem_object_release(&nvbo->bo.base);
> > > kfree(nvbo);
> > > return ret;
> > > }
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, &nvbo);
> > > + drm_gem_object_release(&nvbo->bo.base);
> > > return ret;
> > > }
> > >
> > > --- >8 ---
> > >
> > > Thierry
> >
> > Adding Jeremy for visibility.
> >
>
> Admittedly I only skimmed the code so I'm not extremely confident in my
> analysis, but isn't that handled by the nouveau_bo_del_ttm() callback
> which should get called after the last reference is dropped with
> nouveau_bo_ref?
Yes, it should. But the point here is that we need to get rid of that
nouveau_bo_ref() call to avoid the use-after-free (which is actually
more like a use-before-init in this case, because at this point the
buffer object hasn't been fully initialized yet), so we won't actually
be dropping the reference.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-05-17 15:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 16:35 [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new Karol Herbst
2021-05-13 15:25 ` Jeremy Cline
2021-05-13 17:37 ` Karol Herbst
2021-05-13 18:22 ` Jeremy Cline
2021-05-17 8:56 ` Thierry Reding
2021-05-17 9:19 ` Thierry Reding
2021-05-17 12:28 ` Karol Herbst
2021-05-17 13:32 ` Jeremy Cline
2021-05-17 15:36 ` Thierry Reding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).