* [PATCH] drm/amdgpu: Fix NULL pointer dereference @ 2022-04-08 9:28 Grigory Vasilyev 2022-04-08 10:01 ` Simon Ser 0 siblings, 1 reply; 8+ messages in thread From: Grigory Vasilyev @ 2022-04-08 9:28 UTC (permalink / raw) To: Rodrigo Siqueira, Melissa Wen Cc: Grigory Vasilyev, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Simon Ser, Evan Quan, Bas Nieuwenhuizen, Sean Paul, Qingqing Zhuo, amd-gfx, dri-devel, linux-kernel The code below check for NULL, but is no check at this place, which is potentially dangerous. Signed-off-by: Grigory Vasilyev <h0tc0d3@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index d26810e7311d..c773a92dd4e5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -1017,8 +1017,10 @@ static int amdgpu_display_get_fb_info(const struct amdgpu_framebuffer *amdgpu_fb int r; if (!amdgpu_fb) { - *tiling_flags = 0; - *tmz_surface = false; + if (tiling_flags) + *tiling_flags = 0; + if (tmz_surface) + *tmz_surface = false; return 0; } -- 2.35.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 9:28 [PATCH] drm/amdgpu: Fix NULL pointer dereference Grigory Vasilyev @ 2022-04-08 10:01 ` Simon Ser 2022-04-08 11:28 ` Bas Nieuwenhuizen 0 siblings, 1 reply; 8+ messages in thread From: Simon Ser @ 2022-04-08 10:01 UTC (permalink / raw) To: Grigory Vasilyev Cc: Rodrigo Siqueira, Melissa Wen, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Bas Nieuwenhuizen, Sean Paul, Qingqing Zhuo, amd-gfx, dri-devel, linux-kernel Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? If not, there's no point in adding NULL checks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 10:01 ` Simon Ser @ 2022-04-08 11:28 ` Bas Nieuwenhuizen 2022-04-08 11:48 ` Simon Ser 0 siblings, 1 reply; 8+ messages in thread From: Bas Nieuwenhuizen @ 2022-04-08 11:28 UTC (permalink / raw) To: Simon Ser Cc: Grigory Vasilyev, Rodrigo Siqueira, Melissa Wen, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Sean Paul, Qingqing Zhuo, amd-gfx mailing list, ML dri-devel, LKML On Fri, Apr 8, 2022 at 12:01 PM Simon Ser <contact@emersion.fr> wrote: > > Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > If not, there's no point in adding NULL checks. It isn't called with NULL anywhere, the NULL checks that already exist seem redundant. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 11:28 ` Bas Nieuwenhuizen @ 2022-04-08 11:48 ` Simon Ser 2022-04-08 13:21 ` Grigory Vasilyev 0 siblings, 1 reply; 8+ messages in thread From: Simon Ser @ 2022-04-08 11:48 UTC (permalink / raw) To: Bas Nieuwenhuizen Cc: Grigory Vasilyev, Rodrigo Siqueira, Melissa Wen, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Sean Paul, Qingqing Zhuo, amd-gfx mailing list, ML dri-devel, LKML On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: > > > Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > > If not, there's no point in adding NULL checks. > > It isn't called with NULL anywhere, the NULL checks that already exist > seem redundant. Grigory, would be be willing to submit a v2 which removes the unnecessary NULL checks? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 11:48 ` Simon Ser @ 2022-04-08 13:21 ` Grigory Vasilyev 2022-04-08 13:26 ` Simon Ser 2022-04-08 14:30 ` Christian König 0 siblings, 2 replies; 8+ messages in thread From: Grigory Vasilyev @ 2022-04-08 13:21 UTC (permalink / raw) To: Simon Ser Cc: Bas Nieuwenhuizen, Rodrigo Siqueira, Melissa Wen, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Sean Paul, Qingqing Zhuo, amd-gfx mailing list, ML dri-devel, LKML Simon Ser and Bas Nieuwenhuizen, do you understand that you are proposing to make the code less safe in the future? In the future, someone might rewrite the code and we'll get an error. пт, 8 апр. 2022 г. в 14:48, Simon Ser <contact@emersion.fr>: > > On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > > > On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: > > > > > Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > > > If not, there's no point in adding NULL checks. > > > > It isn't called with NULL anywhere, the NULL checks that already exist > > seem redundant. > > Grigory, would be be willing to submit a v2 which removes the unnecessary > NULL checks? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 13:21 ` Grigory Vasilyev @ 2022-04-08 13:26 ` Simon Ser 2022-04-08 14:30 ` Christian König 1 sibling, 0 replies; 8+ messages in thread From: Simon Ser @ 2022-04-08 13:26 UTC (permalink / raw) To: Grigory Vasilyev Cc: Bas Nieuwenhuizen, Rodrigo Siqueira, Melissa Wen, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Sean Paul, Qingqing Zhuo, amd-gfx mailing list, ML dri-devel, LKML On Friday, April 8th, 2022 at 15:21, Grigory Vasilyev <h0tc0d3@gmail.com> wrote: > Simon Ser and Bas Nieuwenhuizen, do you understand that you are > proposing to make the code less safe in the future? In the future, > someone might rewrite the code and we'll get an error. I don't think we should blindly add NULL checks for all functions which take a pointer as argument. This makes it way more complicated to find a bug when the function is mis-used. Crashing is better because it indicates a programmer error. In the future, any new call with a NULL pointer will produce a clear error. Using pointers for output values is a common pattern in C, it allows a function to return multiple values. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 13:21 ` Grigory Vasilyev 2022-04-08 13:26 ` Simon Ser @ 2022-04-08 14:30 ` Christian König 2022-04-10 0:51 ` Grigory Vasilyev 1 sibling, 1 reply; 8+ messages in thread From: Christian König @ 2022-04-08 14:30 UTC (permalink / raw) To: Grigory Vasilyev, Simon Ser Cc: Bas Nieuwenhuizen, Rodrigo Siqueira, Melissa Wen, Alex Deucher, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Sean Paul, Qingqing Zhuo, amd-gfx mailing list, ML dri-devel, LKML Am 08.04.22 um 15:21 schrieb Grigory Vasilyev: > Simon Ser and Bas Nieuwenhuizen, do you understand that you are > proposing to make the code less safe in the future? In the future, > someone might rewrite the code and we'll get an error. Which is perfectly fine. See error handling is to handle userspace or hardware errors and *not* coding errors. Testing all pointers for NULL without any reason is not defensive, but rather the exactly opposite since it helps hiding real bugs. Regards, Christian. > > пт, 8 апр. 2022 г. в 14:48, Simon Ser <contact@emersion.fr>: >> On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: >> >>> On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: >>> >>>> Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? >>>> If not, there's no point in adding NULL checks. >>> It isn't called with NULL anywhere, the NULL checks that already exist >>> seem redundant. >> Grigory, would be be willing to submit a v2 which removes the unnecessary >> NULL checks? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix NULL pointer dereference 2022-04-08 14:30 ` Christian König @ 2022-04-10 0:51 ` Grigory Vasilyev 0 siblings, 0 replies; 8+ messages in thread From: Grigory Vasilyev @ 2022-04-10 0:51 UTC (permalink / raw) To: Christian König Cc: Simon Ser, Bas Nieuwenhuizen, Rodrigo Siqueira, Melissa Wen, Alex Deucher, Pan, Xinhui, David Airlie, Daniel Vetter, Michel Dänzer, Evan Quan, Sean Paul, Qingqing Zhuo, amd-gfx mailing list, ML dri-devel, LKML Christian König, Simon Ser In fact, the code looks strange, we return the return code, but for some reason we also write false and 0. In my opinion, the caller should do this. Of course, you are right, but I look from the position that nothing should fall in the user system. There may be strange errors that occur periodically under certain conditions and do not affect developers. Also, the user will not always be able to correctly write a bug report. Regards, Grigory. пт, 8 апр. 2022 г. в 17:30, Christian König <christian.koenig@amd.com>: > > Am 08.04.22 um 15:21 schrieb Grigory Vasilyev: > > Simon Ser and Bas Nieuwenhuizen, do you understand that you are > > proposing to make the code less safe in the future? In the future, > > someone might rewrite the code and we'll get an error. > > Which is perfectly fine. > > See error handling is to handle userspace or hardware errors and *not* > coding errors. > > Testing all pointers for NULL without any reason is not defensive, but > rather the exactly opposite since it helps hiding real bugs. > > Regards, > Christian. > > > > > пт, 8 апр. 2022 г. в 14:48, Simon Ser <contact@emersion.fr>: > >> On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > >> > >>> On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: > >>> > >>>> Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > >>>> If not, there's no point in adding NULL checks. > >>> It isn't called with NULL anywhere, the NULL checks that already exist > >>> seem redundant. > >> Grigory, would be be willing to submit a v2 which removes the unnecessary > >> NULL checks? > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-10 0:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-08 9:28 [PATCH] drm/amdgpu: Fix NULL pointer dereference Grigory Vasilyev 2022-04-08 10:01 ` Simon Ser 2022-04-08 11:28 ` Bas Nieuwenhuizen 2022-04-08 11:48 ` Simon Ser 2022-04-08 13:21 ` Grigory Vasilyev 2022-04-08 13:26 ` Simon Ser 2022-04-08 14:30 ` Christian König 2022-04-10 0:51 ` Grigory Vasilyev
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).