From: Felix Kuehling <felix.kuehling@amd.com> To: Grigory Vasilyev <h0tc0d3@gmail.com>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>, Melissa Wen <mwen@igalia.com> Cc: "David Airlie" <airlied@linux.ie>, "Pan, Xinhui" <Xinhui.Pan@amd.com>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, "Alex Deucher" <alexander.deucher@amd.com>, "Christian König" <christian.koenig@amd.com> Subject: Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereference Date: Tue, 5 Apr 2022 13:00:42 -0400 [thread overview] Message-ID: <c55e9866-83f6-a3a0-2ad3-40090e978b40@amd.com> (raw) In-Reply-To: <20220404222132.12740-1-h0tc0d3@gmail.com> Am 2022-04-04 um 18:21 schrieb Grigory Vasilyev: > In the amdgpu_amdkfd_get_xgmi_bandwidth_mbytes function, > the peer_adev pointer can be NULL and is passed to amdgpu_xgmi_get_num_links. > > In amdgpu_xgmi_get_num_links, peer_adev pointer is dereferenced > without any checks: peer_adev->gmc.xgmi.node_id . What's worse, peer_adev is uninitialized with an undefined value if src is NULL. So that code was definitely bogus. However, I think your patch will result in incorrect results. Currently amdgpu_amdkfd_get_xgmi_bandwidth is always called with is_min=true if src==NULL, and with is_min=false if src!=NULL. The intention is, that we assume a single XGMI link in the case that src==NULL. That means the is_min parameter is redundant. What we should do instead is, assume that num_links==1 if src==NULL, and drop the is_min parameter. That would keep things working the way they do now, and prevent potential problems in the future. Regards, Felix > > Signed-off-by: Grigory Vasilyev <h0tc0d3@gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > index be1a55f8b8c5..1a1006b18016 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > @@ -541,11 +541,10 @@ int amdgpu_amdkfd_get_xgmi_bandwidth_mbytes(struct amdgpu_device *dst, > struct amdgpu_device *adev = dst, *peer_adev; > int num_links; > > - if (adev->asic_type != CHIP_ALDEBARAN) > + if (!src || adev->asic_type != CHIP_ALDEBARAN) > return 0; > > - if (src) > - peer_adev = src; > + peer_adev = src; > > /* num links returns 0 for indirect peers since indirect route is unknown. */ > num_links = is_min ? 1 : amdgpu_xgmi_get_num_links(adev, peer_adev);
WARNING: multiple messages have this Message-ID (diff)
From: Felix Kuehling <felix.kuehling@amd.com> To: Grigory Vasilyev <h0tc0d3@gmail.com>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>, Melissa Wen <mwen@igalia.com> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>, linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org, "David Airlie" <airlied@linux.ie>, dri-devel@lists.freedesktop.org, "Alex Deucher" <alexander.deucher@amd.com>, "Christian König" <christian.koenig@amd.com> Subject: Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereference Date: Tue, 5 Apr 2022 13:00:42 -0400 [thread overview] Message-ID: <c55e9866-83f6-a3a0-2ad3-40090e978b40@amd.com> (raw) In-Reply-To: <20220404222132.12740-1-h0tc0d3@gmail.com> Am 2022-04-04 um 18:21 schrieb Grigory Vasilyev: > In the amdgpu_amdkfd_get_xgmi_bandwidth_mbytes function, > the peer_adev pointer can be NULL and is passed to amdgpu_xgmi_get_num_links. > > In amdgpu_xgmi_get_num_links, peer_adev pointer is dereferenced > without any checks: peer_adev->gmc.xgmi.node_id . What's worse, peer_adev is uninitialized with an undefined value if src is NULL. So that code was definitely bogus. However, I think your patch will result in incorrect results. Currently amdgpu_amdkfd_get_xgmi_bandwidth is always called with is_min=true if src==NULL, and with is_min=false if src!=NULL. The intention is, that we assume a single XGMI link in the case that src==NULL. That means the is_min parameter is redundant. What we should do instead is, assume that num_links==1 if src==NULL, and drop the is_min parameter. That would keep things working the way they do now, and prevent potential problems in the future. Regards, Felix > > Signed-off-by: Grigory Vasilyev <h0tc0d3@gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > index be1a55f8b8c5..1a1006b18016 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > @@ -541,11 +541,10 @@ int amdgpu_amdkfd_get_xgmi_bandwidth_mbytes(struct amdgpu_device *dst, > struct amdgpu_device *adev = dst, *peer_adev; > int num_links; > > - if (adev->asic_type != CHIP_ALDEBARAN) > + if (!src || adev->asic_type != CHIP_ALDEBARAN) > return 0; > > - if (src) > - peer_adev = src; > + peer_adev = src; > > /* num links returns 0 for indirect peers since indirect route is unknown. */ > num_links = is_min ? 1 : amdgpu_xgmi_get_num_links(adev, peer_adev);
next prev parent reply other threads:[~2022-04-05 17:00 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-04 22:21 [PATCH] drm/amdkfd: Fix potential NULL pointer dereference Grigory Vasilyev 2022-04-04 22:21 ` Grigory Vasilyev 2022-04-04 22:21 ` Grigory Vasilyev 2022-04-05 17:00 ` Felix Kuehling [this message] 2022-04-05 17:00 ` Felix Kuehling 2022-04-06 13:41 ` Григорий 2022-04-06 13:41 ` Григорий
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=c55e9866-83f6-a3a0-2ad3-40090e978b40@amd.com \ --to=felix.kuehling@amd.com \ --cc=Rodrigo.Siqueira@amd.com \ --cc=Xinhui.Pan@amd.com \ --cc=airlied@linux.ie \ --cc=alexander.deucher@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=christian.koenig@amd.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=h0tc0d3@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mwen@igalia.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.