lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 7 Feb 2024 02:27:27 -0800
From: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
To: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>, Jani Nikula
	<jani.nikula@...ux.intel.com>
CC: Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi
	<rodrigo.vivi@...el.com>, David Airlie <airlied@...il.com>, Daniel Vetter
	<daniel@...ll.ch>, <intel-gfx@...ts.freedesktop.org>,
	<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
	<lvc-project@...uxtesting.org>
Subject: Re: [PATCH] drm/i915/gt: Prevent possible NULL dereference in
 __caps_show()

Hello,

On 2/7/24 01:16, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 06/02/2024 16:45, Nikita Zhandarovich wrote:
>> After falling through the switch statement to default case 'repr' is
>> initialized with NULL, which will lead to incorrect dereference of
>> '!repr[n]' in the following loop.
>>
>> Fix it with the help of an additional check for NULL.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 4ec76dbeb62b ("drm/i915/gt: Expose engine properties via sysfs")
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
>> ---
>> P.S. The NULL-deref problem might be dealt with this way but I am
>> not certain that the rest of the __caps_show() behaviour remains
>> correct if we end up in default case. For instance, as far as I
>> can tell, buf might turn out to be w/o '\0'. I could use some
>> direction if this has to be addressed as well.
>>
>>   drivers/gpu/drm/i915/gt/sysfs_engines.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> index 021f51d9b456..6b130b732867 100644
>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> @@ -105,7 +105,7 @@ __caps_show(struct intel_engine_cs *engine,
>>         len = 0;
>>       for_each_set_bit(n, &caps, show_unknown ? BITS_PER_LONG : count) {
>> -        if (n >= count || !repr[n]) {
>> +        if (n >= count || !repr || !repr[n]) {
> 
> There are two input combinations to this function when repr is NULL.
> 
> First is show_unknown=true and caps=0, which means the for_each_set_bit
> will not execute its body. (No bits set.)
> 
> Second is show_unknown=false and caps=~0, which means count is zero so
> for_each_set_bit will again not run. (Bitfield size input param is zero.)
> 
> So unless I am missing something I do not see the null pointer dereference.
> 
> What could theoretically happen is that a third input combination
> appears, where caps is not zero in the show_unknown=true case, either
> via a fully un-handled engine->class (switch), or a new capability bit
> not added to the static array a bit above.
> 
> That would assert during driver development here:
> 
>             if (GEM_WARN_ON(show_unknown))
> 
> Granted that could be after the dereference in "if (n >= count ||
> !repr[n])", but would be caught in debug builds (CI) and therefore not
> be able to "ship" (get merge to the repo).
> 
> Your second question is about empty buffer returned i.e. len=0 at the
> end of the function? (Which is when the buffer will not be null
> terminated - or you see another option?)
> 
> That I think is safe too since it just results in a zero length read in
> sysfs.
> 
> Regards,
> 
> Tvrtko
> 
>>               if (GEM_WARN_ON(show_unknown))
>>                   len += sysfs_emit_at(buf, len, "[%x] ", n);
>>           } else {

Thank you for such a full response.

I think you are right. I was under the impression that either currently
or in the future there might be an input combination, as you mentioned,
that may trigger the NULL dereference. If you feel it will be caught
beforehand, I am satisfied as well. Same goes for the empty buffer stuff.

I think dropping the patch is the best option then. Apologies for any
inconvenience.

Nikita

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ