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@xxxxxxxxxx>
>> ---
>> 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
