Re: [PATCH] media: v4l: subdev: Prevent NULL routes access

[Cosmin Tanislav <demonsingur@gmail.com>]

On 11/25/24 2:07 PM, Laurent Pinchart wrote:
> On Mon, Nov 25, 2024 at 01:33:15PM +0200, Tomi Valkeinen wrote:
>> On 25/11/2024 10:39, Sakari Ailus wrote:
>>> On Fri, Nov 22, 2024 at 04:37:12PM +0200, Cosmin Tanislav wrote:
>>>> When using v4l2_subdev_set_routing to set a subdev's routing, and the
>>>> passed routing.num_routes is 0, kmemdup is not called to populate the
>>>> routes of the new routing (which is fine, since we wouldn't want to pass
>>>> a possible NULL value to kmemdup).
>>>>
>>>> This results in subdev's routing.routes to be NULL.
>>>>
>>>> routing.routes is further used in some places without being guarded by
>>>> the same num_routes non-zero condition.
>>>>
>>>> Fix it.
>>>
>>> While I think moving the code to copy the routing table seems reasonable,
>>> is there a need to make num_routes == 0 a special case? No memcpy()
>>> implementation should access destination or source if the size is 0.
>>
>> I think so too, but Cosmin convinced me that the spec says otherwise.
>>
>>   From the C spec I have, in "7.21.1 String function conventions":
>>
>> "
>> Where an argument declared as size_t n specifies the length of the array for a
>> function, n can have the value zero on a call to that function. Unless explicitly stated
>> otherwise in the description of a particular function in this subclause, pointer arguments
>> on such a call shall still have valid values, as described in 7.1.4.
>> "
>>
>> The memcpy section has no explicit mention that would hint otherwise.
>>
>> In 7.1.4 Use of library functions it says that unless explicitly stated
>> otherwise, a null pointer is an invalid value.
>>
>> That said, I would still consider memcpy() with size 0 always ok,
>> regardless of the src or dst, as the only memcpy implementation we need
>> to care about is the kernel's.
> 
> I was going to mention that too. The kernel C library API is modeled
> on the standard C library API, but it takes quite a few liberties.
> 
> What I think is important in the context of this patch is to ensure
> consistency in how we model our invariants. I'm less concerned about
> relying on memcpy() being a no-op that doesn't dereference pointers when
> the size is 0 (provided the caller doesn't otherwise trigger C undefined
> behaviours) than about the consistency in how we model routing tables
> with no entry. I'd like to make sure that num_routes == 0 always implies
> routes == NULL and vice versa (which may already be the case, I haven't
> checked).
> 

The following code inside v4l2_subdev_set_routing() assures that
num_routes == 0 results in routing.routes being NULL if num_routes is 0.

if (src->num_routes > 0) {
	new_routing.routes = kmemdup(src->routes, bytes, GFP_KERNEL);
	if (!new_routing.routes)
		return -ENOMEM;
}

Indeed v4l2_subdev_set_routing does not check if routing is NULL before
calling kmemdup on it as far as I can tell.

We should probably introduce a src->routes check in the above code in
the same patch since it already handles NULL access to routes.

We should also not limit src->routes to being NULL if num_routes is
NULL, since it adds unnecessary logic in the caller.