| Message ID | 20241002130149.1607979-1-Igor.A.Artemiev@mcst.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/amd/pm: check return value of amdgpu_irq_add_id() | expand |
Applied. Thanks! Alex On Wed, Oct 2, 2024 at 9:28 AM Igor Artemiev <Igor.A.Artemiev@mcst.ru> wrote: > > amdgpu_irq_ad_id() may fail and the irq handlers will not be registered. > This patch adds error code check. > > Found by Linux Verification Center (linuxtesting.org) with static > analysis tool SVACE. > > Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru> > --- > v2: Remove the cast to struct amdgpu_device as Christian König > <christian.koenig@amd.com> suggested. > > .../drm/amd/pm/powerplay/hwmgr/smu_helper.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > index 79a566f3564a..50a3085c00aa 100644 > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > @@ -647,28 +647,41 @@ int smu9_register_irq_handlers(struct pp_hwmgr *hwmgr) > { > struct amdgpu_irq_src *source = > kzalloc(sizeof(struct amdgpu_irq_src), GFP_KERNEL); > + int ret; > > if (!source) > return -ENOMEM; > > source->funcs = &smu9_irq_funcs; > > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > + ret = amdgpu_irq_add_id(hwmgr->adev, > SOC15_IH_CLIENTID_THM, > THM_9_0__SRCID__THM_DIG_THERM_L2H, > source); > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > + if (ret) > + goto err; > + > + ret = amdgpu_irq_add_id(hwmgr->adev, > SOC15_IH_CLIENTID_THM, > THM_9_0__SRCID__THM_DIG_THERM_H2L, > source); > + if (ret) > + goto err; > > /* Register CTF(GPIO_19) interrupt */ > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > + ret = amdgpu_irq_add_id(hwmgr->adev, > SOC15_IH_CLIENTID_ROM_SMUIO, > SMUIO_9_0__SRCID__SMUIO_GPIO19, > source); > + if (ret) > + goto err; > > return 0; > + > +err: > + kfree(source); > + > + return ret; > } > > void *smu_atom_get_data_table(void *dev, uint32_t table, uint16_t *size, > -- > 2.39.2 >
On Wed, 02. Oct 16:01, Igor Artemiev wrote: > amdgpu_irq_ad_id() may fail and the irq handlers will not be registered. > This patch adds error code check. > > Found by Linux Verification Center (linuxtesting.org) with static > analysis tool SVACE. > > Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru> > --- > v2: Remove the cast to struct amdgpu_device as Christian König > <christian.koenig@amd.com> suggested. > > .../drm/amd/pm/powerplay/hwmgr/smu_helper.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > index 79a566f3564a..50a3085c00aa 100644 > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > @@ -647,28 +647,41 @@ int smu9_register_irq_handlers(struct pp_hwmgr *hwmgr) > { > struct amdgpu_irq_src *source = > kzalloc(sizeof(struct amdgpu_irq_src), GFP_KERNEL); > + int ret; > > if (!source) > return -ENOMEM; > > source->funcs = &smu9_irq_funcs; > > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > + ret = amdgpu_irq_add_id(hwmgr->adev, > SOC15_IH_CLIENTID_THM, > THM_9_0__SRCID__THM_DIG_THERM_L2H, > source); > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > + if (ret) > + goto err; > + > + ret = amdgpu_irq_add_id(hwmgr->adev, > SOC15_IH_CLIENTID_THM, > THM_9_0__SRCID__THM_DIG_THERM_H2L, > source); > + if (ret) > + goto err; > > /* Register CTF(GPIO_19) interrupt */ > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > + ret = amdgpu_irq_add_id(hwmgr->adev, > SOC15_IH_CLIENTID_ROM_SMUIO, > SMUIO_9_0__SRCID__SMUIO_GPIO19, > source); > + if (ret) > + goto err; > > return 0; > + > +err: > + kfree(source); Oh, the calltrace looks like: hwmgr_sw_init() phm_register_irq_handlers() ->register_irq_handlers() smu9_register_irq_handlers() And the return value of phm_register_irq_handlers() is not processed and the error is not reported anywhere, so I guess there is a risk of use-after-free: the source pointer may have been already registered by some of amdgpu_irq_add_id() calls before the error occured. The similar code exists in smu7_register_irq_handlers(), maybe should be fixed as well. Alex, is https://gitlab.freedesktop.org/agd5f/linux a public repo this patch should go in? I'd suggest to drop the patch and ask Igor to do a complete fix or, if dropping is not possible now, fix it by another patch. For the latter one I can do this myself but it would be nice to refer to the current patch via a git hash (it's probably not published yet in your repo). > + > + return ret; > } > > void *smu_atom_get_data_table(void *dev, uint32_t table, uint16_t *size, > -- > 2.39.2
On Thu, Oct 3, 2024 at 10:28 AM Fedor Pchelkin <pchelkin@ispras.ru> wrote: > > On Wed, 02. Oct 16:01, Igor Artemiev wrote: > > amdgpu_irq_ad_id() may fail and the irq handlers will not be registered. > > This patch adds error code check. > > > > Found by Linux Verification Center (linuxtesting.org) with static > > analysis tool SVACE. > > > > Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru> > > --- > > v2: Remove the cast to struct amdgpu_device as Christian König > > <christian.koenig@amd.com> suggested. > > > > .../drm/amd/pm/powerplay/hwmgr/smu_helper.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > > index 79a566f3564a..50a3085c00aa 100644 > > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c > > @@ -647,28 +647,41 @@ int smu9_register_irq_handlers(struct pp_hwmgr *hwmgr) > > { > > struct amdgpu_irq_src *source = > > kzalloc(sizeof(struct amdgpu_irq_src), GFP_KERNEL); > > + int ret; > > > > if (!source) > > return -ENOMEM; > > > > source->funcs = &smu9_irq_funcs; > > > > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > > + ret = amdgpu_irq_add_id(hwmgr->adev, > > SOC15_IH_CLIENTID_THM, > > THM_9_0__SRCID__THM_DIG_THERM_L2H, > > source); > > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > > + if (ret) > > + goto err; > > + > > + ret = amdgpu_irq_add_id(hwmgr->adev, > > SOC15_IH_CLIENTID_THM, > > THM_9_0__SRCID__THM_DIG_THERM_H2L, > > source); > > + if (ret) > > + goto err; > > > > /* Register CTF(GPIO_19) interrupt */ > > - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), > > + ret = amdgpu_irq_add_id(hwmgr->adev, > > SOC15_IH_CLIENTID_ROM_SMUIO, > > SMUIO_9_0__SRCID__SMUIO_GPIO19, > > source); > > + if (ret) > > + goto err; > > > > return 0; > > + > > +err: > > + kfree(source); > > Oh, the calltrace looks like: > > hwmgr_sw_init() > phm_register_irq_handlers() > ->register_irq_handlers() > smu9_register_irq_handlers() > > And the return value of phm_register_irq_handlers() is not processed and > the error is not reported anywhere, so I guess there is a risk of > use-after-free: the source pointer may have been already registered by > some of amdgpu_irq_add_id() calls before the error occured. > > The similar code exists in smu7_register_irq_handlers(), maybe should be > fixed as well. > > Alex, is https://gitlab.freedesktop.org/agd5f/linux a public repo this > patch should go in? I'd suggest to drop the patch and ask Igor to do a > complete fix or, if dropping is not possible now, fix it by another patch. > For the latter one I can do this myself but it would be nice to refer to > the current patch via a git hash (it's probably not published yet in your > repo). Dropped. Thanks. Alex > > > + > > + return ret; > > } > > > > void *smu_atom_get_data_table(void *dev, uint32_t table, uint16_t *size, > > -- > > 2.39.2
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c index 79a566f3564a..50a3085c00aa 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c @@ -647,28 +647,41 @@ int smu9_register_irq_handlers(struct pp_hwmgr *hwmgr) { struct amdgpu_irq_src *source = kzalloc(sizeof(struct amdgpu_irq_src), GFP_KERNEL); + int ret; if (!source) return -ENOMEM; source->funcs = &smu9_irq_funcs; - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), + ret = amdgpu_irq_add_id(hwmgr->adev, SOC15_IH_CLIENTID_THM, THM_9_0__SRCID__THM_DIG_THERM_L2H, source); - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), + if (ret) + goto err; + + ret = amdgpu_irq_add_id(hwmgr->adev, SOC15_IH_CLIENTID_THM, THM_9_0__SRCID__THM_DIG_THERM_H2L, source); + if (ret) + goto err; /* Register CTF(GPIO_19) interrupt */ - amdgpu_irq_add_id((struct amdgpu_device *)(hwmgr->adev), + ret = amdgpu_irq_add_id(hwmgr->adev, SOC15_IH_CLIENTID_ROM_SMUIO, SMUIO_9_0__SRCID__SMUIO_GPIO19, source); + if (ret) + goto err; return 0; + +err: + kfree(source); + + return ret; } void *smu_atom_get_data_table(void *dev, uint32_t table, uint16_t *size,
amdgpu_irq_ad_id() may fail and the irq handlers will not be registered. This patch adds error code check. Found by Linux Verification Center (linuxtesting.org) with static analysis tool SVACE. Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru> --- v2: Remove the cast to struct amdgpu_device as Christian König <christian.koenig@amd.com> suggested. .../drm/amd/pm/powerplay/hwmgr/smu_helper.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)