[PATCH] iosys-map: Fix undefined behavior in iosys_map_clear()

[Thomas Zimmermann]

Hi

Am 30.07.25 um 14:51 schrieb Gote, Nitin R:
> Hi,
>
> The patch has been reviewed and approved.
> Could someone please help to merge it?

Merged into drm-misc-fixes now, sorry for the delay.

Best regards
Thomas

>
> Thank you,
> Nitin
>
>> -----Original Message-----
>> From: Gote, Nitin R
>> Sent: Thursday, July 24, 2025 10:36 AM
>> To: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Shyti, Andi
>> <Andi.Shyti at intel.com>
>> Subject: RE: [PATCH] iosys-map: Fix undefined behavior in iosys_map_clear()
>>
>> Hi Thomas,
>>
>>> Hi
>>>
>>> Am 18.07.25 um 16:47 schrieb Andi Shyti:
>>>> Hi Nitin,
>>>>
>>>> On Fri, Jul 18, 2025 at 04:20:51PM +0530, Nitin Gote wrote:
>>>>> The current iosys_map_clear() implementation reads the potentially
>>>>> uninitialized 'is_iomem' boolean field to decide which union member
>>>>> to clear. This causes undefined behavior when called on
>>>>> uninitialized structures, as 'is_iomem' may contain garbage values like 0xFF.
>>>>>
>>>>> UBSAN detects this as:
>>>>>       UBSAN: invalid-load in include/linux/iosys-map.h:267
>>>>>       load of value 255 is not a valid value for type '_Bool'
>>>>>
>>>>> Fix by unconditionally clearing the entire structure with memset(),
>>>>> eliminating the need to read uninitialized data and ensuring all
>>>>> fields are set to known good values.
>>>>>
>>>>> Closes:
>>>>> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14639
>>>>> Fixes: 01fd30da0474 ("dma-buf: Add struct dma-buf-map for storing
>>>>> struct dma_buf.vaddr_ptr")
>>>>> Signed-off-by: Nitin Gote <nitin.r.gote at intel.com>
>>>> +Thomas and the dri-devel mailing list.
>>>>
>>>> In any case, your patch makes sense to me:
>>> The call to iosys_map_clear() is at
>>>
>>> https://elixir.bootlin.com/linux/v6.15.6/source/drivers/dma-buf/dma-
>>> buf.c#L1571
>>>
>>> It's a defensive measure for cases where the caller reads the returned
>>> map address when it was never initialized by the vmap implementation.
>>> I'm not a big fan of memset(), but OK. Preferably, iosys_map_clear()
>>> would simply set vaddr = NULL and is_iomem = false. Anyway,
>>>
>>> Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>
>>> Best regards
>>> Thomas
>>>
>> Thank you for the review.
>> Could you please help to merge this patch?
>>
>> - Nitin
>>
>>>> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
>>>>
>>>> Andi
>>>>
>>>>> ---
>>>>>    include/linux/iosys-map.h | 7 +------
>>>>>    1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>>>>> index 4696abfd311c..3e85afe794c0 100644
>>>>> --- a/include/linux/iosys-map.h
>>>>> +++ b/include/linux/iosys-map.h
>>>>> @@ -264,12 +264,7 @@ static inline bool iosys_map_is_set(const
>>>>> struct
>>> iosys_map *map)
>>>>>     */
>>>>>    static inline void iosys_map_clear(struct iosys_map *map)
>>>>>    {
>>>>> -	if (map->is_iomem) {
>>>>> -		map->vaddr_iomem = NULL;
>>>>> -		map->is_iomem = false;
>>>>> -	} else {
>>>>> -		map->vaddr = NULL;
>>>>> -	}
>>>>> +	memset(map, 0, sizeof(*map));
>>>>>    }
>>>>>
>>>>>    /**
>>>>> --
>>>>> 2.25.1
>>> --
>>> --
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Software Solutions Germany GmbH
>>> Frankenstrasse 146, 90461 Nuernberg, Germany
>>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB
>>> 36809 (AG Nuernberg)

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)