| Message ID | 20210910100922.12097-1-guchun.chen@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ttm: add a WARN_ON in ttm_set_driver_manager when array bounds (v2) | expand |
On 2021-09-10 11:09, Guchun Chen wrote: > Vendor will define their own memory types on top of TTM_PL_PRIV, > but call ttm_set_driver_manager directly without checking mem_type > value when setting up memory manager. So add such check to aware > the case when array bounds. > > v2: lower check level to WARN_ON > > Signed-off-by: Leslie Shi <Yuliang.Shi@amd.com> > Signed-off-by: Guchun Chen <guchun.chen@amd.com> > --- > include/drm/ttm/ttm_device.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/drm/ttm/ttm_device.h b/include/drm/ttm/ttm_device.h > index 07d722950d5b..aa79953c807c 100644 > --- a/include/drm/ttm/ttm_device.h > +++ b/include/drm/ttm/ttm_device.h > @@ -291,6 +291,7 @@ ttm_manager_type(struct ttm_device *bdev, int mem_type) > static inline void ttm_set_driver_manager(struct ttm_device *bdev, int type, > struct ttm_resource_manager *manager) > { > + WARN_ON(type >= TTM_NUM_MEM_TYPES); Nit: I know nothing about this code, but from the context alone it would seem sensible to do if (WARN_ON(type >= TTM_NUM_MEM_TYPES)) return; to avoid making the subsequent assignment when we *know* it's invalid and likely to corrupt memory. Robin. > bdev->man_drv[type] = manager; > } > >
[Public] Thanks for your suggestion, Robin. Do you agree with this as well, Christian and Xinhui? Regards, Guchun -----Original Message----- From: Robin Murphy <robin.murphy@arm.com> Sent: Saturday, September 11, 2021 2:25 AM To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com> Cc: Shi, Leslie <Yuliang.Shi@amd.com> Subject: Re: [PATCH] drm/ttm: add a WARN_ON in ttm_set_driver_manager when array bounds (v2) On 2021-09-10 11:09, Guchun Chen wrote: > Vendor will define their own memory types on top of TTM_PL_PRIV, but > call ttm_set_driver_manager directly without checking mem_type value > when setting up memory manager. So add such check to aware the case > when array bounds. > > v2: lower check level to WARN_ON > > Signed-off-by: Leslie Shi <Yuliang.Shi@amd.com> > Signed-off-by: Guchun Chen <guchun.chen@amd.com> > --- > include/drm/ttm/ttm_device.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/drm/ttm/ttm_device.h > b/include/drm/ttm/ttm_device.h index 07d722950d5b..aa79953c807c 100644 > --- a/include/drm/ttm/ttm_device.h > +++ b/include/drm/ttm/ttm_device.h > @@ -291,6 +291,7 @@ ttm_manager_type(struct ttm_device *bdev, int mem_type) > static inline void ttm_set_driver_manager(struct ttm_device *bdev, int type, > struct ttm_resource_manager *manager) > { > + WARN_ON(type >= TTM_NUM_MEM_TYPES); Nit: I know nothing about this code, but from the context alone it would seem sensible to do if (WARN_ON(type >= TTM_NUM_MEM_TYPES)) return; to avoid making the subsequent assignment when we *know* it's invalid and likely to corrupt memory. Robin. > bdev->man_drv[type] = manager; > } > >
Well it will crash later on when accessing the invalid offset, so not much gained. But either way works for me. Christian. Am 13.09.21 um 04:35 schrieb Chen, Guchun: > [Public] > > Thanks for your suggestion, Robin. Do you agree with this as well, Christian and Xinhui? > > Regards, > Guchun > > -----Original Message----- > From: Robin Murphy <robin.murphy@arm.com> > Sent: Saturday, September 11, 2021 2:25 AM > To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com> > Cc: Shi, Leslie <Yuliang.Shi@amd.com> > Subject: Re: [PATCH] drm/ttm: add a WARN_ON in ttm_set_driver_manager when array bounds (v2) > > On 2021-09-10 11:09, Guchun Chen wrote: >> Vendor will define their own memory types on top of TTM_PL_PRIV, but >> call ttm_set_driver_manager directly without checking mem_type value >> when setting up memory manager. So add such check to aware the case >> when array bounds. >> >> v2: lower check level to WARN_ON >> >> Signed-off-by: Leslie Shi <Yuliang.Shi@amd.com> >> Signed-off-by: Guchun Chen <guchun.chen@amd.com> >> --- >> include/drm/ttm/ttm_device.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/drm/ttm/ttm_device.h >> b/include/drm/ttm/ttm_device.h index 07d722950d5b..aa79953c807c 100644 >> --- a/include/drm/ttm/ttm_device.h >> +++ b/include/drm/ttm/ttm_device.h >> @@ -291,6 +291,7 @@ ttm_manager_type(struct ttm_device *bdev, int mem_type) >> static inline void ttm_set_driver_manager(struct ttm_device *bdev, int type, >> struct ttm_resource_manager *manager) >> { >> + WARN_ON(type >= TTM_NUM_MEM_TYPES); > Nit: I know nothing about this code, but from the context alone it would seem sensible to do > > if (WARN_ON(type >= TTM_NUM_MEM_TYPES)) > return; > > to avoid making the subsequent assignment when we *know* it's invalid and likely to corrupt memory. > > Robin. > >> bdev->man_drv[type] = manager; >> } >> >>
diff --git a/include/drm/ttm/ttm_device.h b/include/drm/ttm/ttm_device.h index 07d722950d5b..aa79953c807c 100644 --- a/include/drm/ttm/ttm_device.h +++ b/include/drm/ttm/ttm_device.h @@ -291,6 +291,7 @@ ttm_manager_type(struct ttm_device *bdev, int mem_type) static inline void ttm_set_driver_manager(struct ttm_device *bdev, int type, struct ttm_resource_manager *manager) { + WARN_ON(type >= TTM_NUM_MEM_TYPES); bdev->man_drv[type] = manager; }