From: Danilo Krummrich <dakr@redhat.com>
To: Yuran Pereira <yuran.pereira@hotmail.com>, airlied@gmail.com
Cc: nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, sumit.semwal@linaro.org,
linaro-mm-sig@lists.linaro.org, daniel@ffwll.ch,
linux-kernel-mentees@lists.linuxfoundation.org,
christian.koenig@amd.com, linux-media@vger.kernel.org
Subject: Re: [Nouveau] [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare
Date: Tue, 14 Nov 2023 17:23:59 +0100 [thread overview]
Message-ID: <6a1ebcef-bade-45a0-9bd9-c05f0226eb88@redhat.com> (raw)
In-Reply-To: <DB3PR10MB6835FA6E15F3C830FC793D2EE8DDA@DB3PR10MB6835.EURPRD10.PROD.OUTLOOK.COM>
Hi Yuran,
On 10/26/23 19:03, Yuran Pereira wrote:
> There are instances where the "args" argument passed to
> nouveau_uvmm_sm_prepare() is NULL.
>
> I.e. when nouveau_uvmm_sm_prepare() is called from
> nouveau_uvmm_sm_unmap_prepare()
>
> ```
> static int
> nouveau_uvmm_sm_unmap_prepare(struct nouveau_uvmm *uvmm,
> ...
> {
> return nouveau_uvmm_sm_prepare(uvmm, new, ops, NULL);
> }
> ```
>
> The problem is that op_map_prepare() which nouveau_uvmm_sm_prepare
> calls, dereferences this value, which can lead to a NULL pointer
> dereference.
op_map_prepare() can't be called with `args` being NULL, since when called
through nouveau_uvmm_sm_unmap_prepare() we can't hit the DRM_GPUVA_OP_MAP
case at all.
Unmapping something never leads to a new mapping being created, it can lead
to remaps though.
>
> ```
> static int
> op_map_prepare(struct nouveau_uvmm *uvmm,
> ...
> {
> ...
> uvma->region = args->region; <-- Dereferencing of possibly NULL pointer
> uvma->kind = args->kind; <--
> ...
> }
> ```
>
> ```
> static int
> nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
> ...
> struct uvmm_map_args *args)
> {
> struct drm_gpuva_op *op;
> u64 vmm_get_start = args ? args->addr : 0;
> u64 vmm_get_end = args ? args->addr + args->range : 0;
> int ret;
>
> drm_gpuva_for_each_op(op, ops) {
> switch (op->op) {
> case DRM_GPUVA_OP_MAP: {
> u64 vmm_get_range = vmm_get_end - vmm_get_start;
>
> ret = op_map_prepare(uvmm, &new->map, &op->map, args); <---
> if (ret)
> goto unwind;
>
> if (args && vmm_get_range) {
> ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start,
> vmm_get_range);
> if (ret) {
> op_map_prepare_unwind(new->map);
> goto unwind;
> }
> }
> ...
> ```
>
> Since the switch "case DRM_GPUVA_OP_MAP", also NULL checks "args"
This check is not required for the reason given above. If you like, you
can change this patch up to remove the args check and add a comment like:
/* args can't be NULL when called for a map operation. */
> after the call to op_map_prepare(), my guess is that we should
> probably relocate this check to a point before op_map_prepare()
> is called.
Yeah, I see how this unnecessary check made you think so.
- Danilo
>
> This patch ensures that the value of args is checked before
> calling op_map_prepare()
>
> Addresses-Coverity-ID: 1544574 ("Dereference after null check")
> Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
> ---
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index aae780e4a4aa..6baa481eb2c8 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -620,11 +620,14 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
> case DRM_GPUVA_OP_MAP: {
> u64 vmm_get_range = vmm_get_end - vmm_get_start;
>
> + if (!args)
> + goto unwind;
> +
> ret = op_map_prepare(uvmm, &new->map, &op->map, args);
> if (ret)
> goto unwind;
>
> - if (args && vmm_get_range) {
> + if (vmm_get_range) {
> ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start,
> vmm_get_range);
> if (ret) {
next prev parent reply other threads:[~2023-11-14 16:24 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-26 17:03 [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare Yuran Pereira
2023-11-14 16:23 ` Danilo Krummrich [this message]
2023-11-15 9:08 ` Yuran Pereira
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6a1ebcef-bade-45a0-9bd9-c05f0226eb88@redhat.com \
--to=dakr@redhat.com \
--cc=airlied@gmail.com \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=sumit.semwal@linaro.org \
--cc=yuran.pereira@hotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).