Hi Thomas, Apologies for the noise, please disregard my previous email. I spoke too soon.
After re-evaluating the C standard integer promotion rules, you are absolutely right. Since `args->new_handle` is unsigned, `INT_MAX` is promoted to unsigned during the comparison. A negative payload from the fuzzer (e.g., 0xfffffffd) actually evaluates as a massive positive number and is correctly caught by the original `> INT_MAX` check. The implicit conversion bypass I initially suspected does not happen here. However, your concern about the `handle + 1` overflow is the real issue. If the user passes exactly `INT_MAX`, it bypasses the `>` check, and `INT_MAX + 1` overflows to a negative value when evaluated for `idr_alloc()`. While `idr_alloc()` handles negative end limits by falling back to `INT_MAX`, this breaks the exact-ID allocation semantics intended by the caller. I have adjusted the patch to focus entirely on preventing this signed integer overflow by tightening the check to `>= INT_MAX`. I will send the v2 patch shortly. Thanks, Mingyu > -----原始邮件----- > 发件人: 王明煜 <[email protected]> > 发送时间:2026-06-05 21:38:27 (星期五) > 收件人: "Thomas Zimmermann" <[email protected]> > 抄送: [email protected], [email protected], [email protected], > [email protected], [email protected], [email protected] > 主题: Re: Re: [PATCH] drm/gem: fix warning in idr_alloc due to unvalidated user > handle > > Hi Thomas, > > Thank you for the insightful review and suggestions. You were absolutely > right on both points. > > I went back to analyze the Syzkaller reproducer trace. The fuzzer actually > managed to pass 0xfffffffd as args->new_handle. Due to the explicit (int) > cast in the INT_MAX macro definition, an implicit type conversion quirk > allowed this value to bypass the args->new_handle > INT_MAX check, ultimately > reaching idr_alloc as -3 and triggering the warning. > > Testing your suggestion to explicitly cast INT_MAX to u32 successfully > catches this and prevents the warning. > > Regarding your second concern about handle + 1, I completely agree. If handle > equals INT_MAX, handle + 1 will overflow to a negative value. Although > idr_alloc handles negative end values by falling back to INT_MAX, it breaks > the exact-ID allocation logic intended here. > > I will send a v2 patch shortly that explicitly casts INT_MAX and tightens the > boundary check to >= to cover the handle + 1 overflow. > > Thanks, > Mingyu > > > > -----原始邮件----- > > 发件人: "Thomas Zimmermann" <[email protected]> > > 发送时间:2026-06-05 16:02:33 (星期五) > > 收件人: 王明煜 <[email protected]>, > > [email protected], [email protected], [email protected], > > [email protected] > > 抄送: [email protected], [email protected] > > 主题: Re: [PATCH] drm/gem: fix warning in idr_alloc due to unvalidated user > > handle > > > > Hi > > > > Am 05.06.26 um 05:06 schrieb 王明煜: > > > Hi maintainers, > > > > > > Just a gentle ping on this patch. It fixes a WARN triggered during > > > fuzzing when negative user handles are passed. > > > > > > Please let me know if it needs any revisions or if there is anything else > > > I can do to help move it forward. > > > > > > Thanks, > > > Mingyu > > > > > > > > >> -----原始邮件----- > > >> 发件人: "Mingyu Wang" <[email protected]> > > >> 发送时间:2026-04-22 19:42:47 (星期三) > > >> 收件人: [email protected], [email protected], > > >> [email protected], [email protected], [email protected] > > >> 抄送: [email protected], [email protected], > > >> "Mingyu Wang" <[email protected]> > > >> 主题: [PATCH] drm/gem: fix warning in idr_alloc due to unvalidated user > > >> handle > > >> > > >> During fuzzing, a warning was triggered in idr_alloc() when handling > > >> the DRM_IOCTL_GEM_CHANGE_HANDLE (or similar) ioctl. > > >> > > >> The function drm_gem_change_handle_ioctl() currently only checks if > > >> args->new_handle is strictly greater than INT_MAX. However, it fails > > >> to check for negative values. If a userpace application passes a > > >> negative handle, it bypasses the upper-bound check and is passed > > >> directly to idr_alloc() as the 'start' parameter, triggering the > > >> WARN_ON_ONCE(start < 0) inside idr_alloc(). > > > > args->new_handle is unsigned. IIRC, for the test, INT_MAX should be > > interpreted as unsigned as well. So how can it get across the INT_MAX > > test? There's an explicit cast to int at [1], which might have an effect > > here. > > > > Does it work of you explicitly cast INT_MAX to u32 in that test? > > > > I'm also worried about interpreting the handle as signed and then adding > > +1 to it. [2] idr_alloc() appears to handle it gracefully, [3] but it > > still looks fishy. > > > > [1] https://elixir.bootlin.com/linux/v7.0.11/source/include/vdso/limits.h#L8 > > [2] > > https://elixir.bootlin.com/linux/v7.0.11/source/drivers/gpu/drm/drm_gem.c#L1033 > > [3] https://elixir.bootlin.com/linux/v7.0.11/source/lib/idr.c#L89 > > > > Best regards > > Thomas > > > > >> > > >> Fix this by explicitly validating that the user-provided handle is > > >> strictly positive and within the valid IDR range. > > >> > > >> Signed-off-by: Mingyu Wang <[email protected]> > > >> --- > > >> drivers/gpu/drm/drm_gem.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > >> index d6424267260b..3d84d4f1c3e0 100644 > > >> --- a/drivers/gpu/drm/drm_gem.c > > >> +++ b/drivers/gpu/drm/drm_gem.c > > >> @@ -1026,7 +1026,7 @@ int drm_gem_change_handle_ioctl(struct drm_device > > >> *dev, void *data, > > >> return -EOPNOTSUPP; > > >> > > >> /* idr_alloc() limitation. */ > > >> - if (args->new_handle > INT_MAX) > > >> + if (args->new_handle <= 0 || args->new_handle > INT_MAX) > > >> return -EINVAL; > > >> handle = args->new_handle; > > >> > > >> -- > > >> 2.34.1 > > > > -- > > -- > > Thomas Zimmermann > > Graphics Driver Developer > > SUSE Software Solutions Germany GmbH > > Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com > > GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg) > >
