| Message ID | 20241122143717.173344-1-demonsingur@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l: subdev: Prevent NULL routes access | expand |
Hi, On 25/11/2024 10:39, Sakari Ailus wrote: > Hi Cosmin, > > Thanks for the patch. > > 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. Tomi
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).
On 11/24/24 8:49 AM, Laurent Pinchart 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. > > What other places is that ? Have you experienced a crash anywhere ? > The other places are exactly the ones being fixed in this patch. I can probably reword it if it's unclear. >> >> Fix it. >> > > A Fixes: tag would be good to help backporting. > >> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 46 +++++++++++++-------------- >> 1 file changed, 23 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index cde1774c9098d..4f0eecd7fd66f 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -605,6 +605,23 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh, >> v4l2_subdev_get_unlocked_active_state(sd); >> } >> >> +static void subdev_copy_krouting(struct v4l2_subdev_routing *routing, >> + struct v4l2_subdev_krouting *krouting) > > The second argument should be const. > Will do for V2. >> +{ >> + memset(routing->reserved, 0, sizeof(routing->reserved)); >> + >> + if (!krouting->routes) { >> + routing->num_routes = 0; >> + return; >> + } >> + >> + memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, >> + krouting->routes, >> + min(krouting->num_routes, routing->len_routes) * >> + sizeof(*krouting->routes)); >> + routing->num_routes = krouting->num_routes; >> +} >> + >> static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> struct v4l2_subdev_state *state) >> { >> @@ -976,7 +993,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> >> case VIDIOC_SUBDEV_G_ROUTING: { >> struct v4l2_subdev_routing *routing = arg; >> - struct v4l2_subdev_krouting *krouting; >> >> if (!v4l2_subdev_enable_streams_api) >> return -ENOIOCTLCMD; >> @@ -984,15 +1000,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) >> return -ENOIOCTLCMD; >> >> - memset(routing->reserved, 0, sizeof(routing->reserved)); >> - >> - krouting = &state->routing; >> - >> - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, >> - krouting->routes, >> - min(krouting->num_routes, routing->len_routes) * >> - sizeof(*krouting->routes)); >> - routing->num_routes = krouting->num_routes; >> + subdev_copy_krouting(routing, &state->routing); >> >> return 0; >> } >> @@ -1016,8 +1024,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> if (routing->num_routes > routing->len_routes) >> return -EINVAL; >> >> - memset(routing->reserved, 0, sizeof(routing->reserved)); >> - >> for (i = 0; i < routing->num_routes; ++i) { >> const struct v4l2_subdev_route *route = &routes[i]; >> const struct media_pad *pads = sd->entity.pads; >> @@ -1046,12 +1052,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> * the routing table. >> */ >> if (!v4l2_subdev_has_op(sd, pad, set_routing)) { >> - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, >> - state->routing.routes, >> - min(state->routing.num_routes, routing->len_routes) * >> - sizeof(*state->routing.routes)); >> - routing->num_routes = state->routing.num_routes; >> - >> + subdev_copy_krouting(routing, &state->routing); >> return 0; >> } >> >> @@ -1064,11 +1065,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> if (rval < 0) >> return rval; >> >> - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, >> - state->routing.routes, >> - min(state->routing.num_routes, routing->len_routes) * >> - sizeof(*state->routing.routes)); >> - routing->num_routes = state->routing.num_routes; >> + subdev_copy_krouting(routing, &state->routing); >> >> return 0; >> } >> @@ -1956,6 +1953,9 @@ struct v4l2_subdev_route * >> __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, >> struct v4l2_subdev_route *route) >> { >> + if (!routing->routes) >> + return NULL; >> + >> if (route) >> ++route; >> else >
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.
Hi Cosmin, On Mon, Nov 25, 2024 at 08:42:09PM +0200, Cosmin Tanislav wrote: > > > 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. I'd keep that out of this patch. Beyond that, v4l2_subdev_routing_validate() is generally called before v4l2_subdev_set_routing() and that function doesn't have the check either. I think it should be added there. Of course triggering it requires a driver (or framework) bug so it's just a sanity check. > > We should also not limit src->routes to being NULL if num_routes is > NULL, since it adds unnecessary logic in the caller.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index cde1774c9098d..4f0eecd7fd66f 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -605,6 +605,23 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh, v4l2_subdev_get_unlocked_active_state(sd); } +static void subdev_copy_krouting(struct v4l2_subdev_routing *routing, + struct v4l2_subdev_krouting *krouting) +{ + memset(routing->reserved, 0, sizeof(routing->reserved)); + + if (!krouting->routes) { + routing->num_routes = 0; + return; + } + + memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, + krouting->routes, + min(krouting->num_routes, routing->len_routes) * + sizeof(*krouting->routes)); + routing->num_routes = krouting->num_routes; +} + static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, struct v4l2_subdev_state *state) { @@ -976,7 +993,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_G_ROUTING: { struct v4l2_subdev_routing *routing = arg; - struct v4l2_subdev_krouting *krouting; if (!v4l2_subdev_enable_streams_api) return -ENOIOCTLCMD; @@ -984,15 +1000,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) return -ENOIOCTLCMD; - memset(routing->reserved, 0, sizeof(routing->reserved)); - - krouting = &state->routing; - - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, - krouting->routes, - min(krouting->num_routes, routing->len_routes) * - sizeof(*krouting->routes)); - routing->num_routes = krouting->num_routes; + subdev_copy_krouting(routing, &state->routing); return 0; } @@ -1016,8 +1024,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (routing->num_routes > routing->len_routes) return -EINVAL; - memset(routing->reserved, 0, sizeof(routing->reserved)); - for (i = 0; i < routing->num_routes; ++i) { const struct v4l2_subdev_route *route = &routes[i]; const struct media_pad *pads = sd->entity.pads; @@ -1046,12 +1052,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, * the routing table. */ if (!v4l2_subdev_has_op(sd, pad, set_routing)) { - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, - state->routing.routes, - min(state->routing.num_routes, routing->len_routes) * - sizeof(*state->routing.routes)); - routing->num_routes = state->routing.num_routes; - + subdev_copy_krouting(routing, &state->routing); return 0; } @@ -1064,11 +1065,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (rval < 0) return rval; - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, - state->routing.routes, - min(state->routing.num_routes, routing->len_routes) * - sizeof(*state->routing.routes)); - routing->num_routes = state->routing.num_routes; + subdev_copy_krouting(routing, &state->routing); return 0; } @@ -1956,6 +1953,9 @@ struct v4l2_subdev_route * __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, struct v4l2_subdev_route *route) { + if (!routing->routes) + return NULL; + if (route) ++route; else
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. Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 46 +++++++++++++-------------- 1 file changed, 23 insertions(+), 23 deletions(-)