[PATCH] Block: Fix block/elevator.c elevator_get() off-by-one error

[PATCH] Block: Fix block/elevator.c elevator_get() off-by-one error

[wzt.wzt]

elevator_get() not check the name length, if the name length > sizeof(elv),
elv will miss the '\0'. And elv buffer will be replace "-iosched" as something
like aaaaaaaaa, then call request_module() can load an not trust module.

Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>

---
 block/elevator.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index df75676..76e3702 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -154,7 +154,7 @@ static struct elevator_type *elevator_get(const char *name)
 
 		spin_unlock(&elv_list_lock);
 
-		sprintf(elv, "%s-iosched", name);
+		snprintf(elv, sizeof(elv), "%s-iosched", name);
 
 		request_module("%s", elv);
 		spin_lock(&elv_list_lock);
-- 
1.6.5.3

Re: [PATCH] Block: Fix block/elevator.c elevator_get() off-by-one error

[Jens Axboe]

On Tue, Mar 30 2010, wzt.wzt@gmail.com wrote:
> elevator_get() not check the name length, if the name length > sizeof(elv),
> elv will miss the '\0'. And elv buffer will be replace "-iosched" as something
> like aaaaaaaaa, then call request_module() can load an not trust module.

Thanks, good catch! Applied.


-- 
Jens Axboe

Re: [PATCH] Block: Fix block/elevator.c elevator_get() off-by-one error

[Xiaotian Feng]

On Tue, Mar 30, 2010 at 5:21 PM,  <wzt.wzt@gmail.com> wrote:
> elevator_get() not check the name length, if the name length > sizeof(elv),
> elv will miss the '\0'. And elv buffer will be replace "-iosched" as something
> like aaaaaaaaa, then call request_module() can load an not trust module.
>
> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
>
> ---
>  block/elevator.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block/elevator.c b/block/elevator.c
> index df75676..76e3702 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -154,7 +154,7 @@ static struct elevator_type *elevator_get(const char *name)
>
>                spin_unlock(&elv_list_lock);
>
> -               sprintf(elv, "%s-iosched", name);
> +               snprintf(elv, sizeof(elv), "%s-iosched", name);
>

elv is defined as char elv[ELV_NAME_MAX + strlen("-iosched")];
so if name length > sizeof(elv), the name length must already bigger
than ELV_NAME_MAX

elevator_get is used in elevator_init, so if elevator_init is passing
a super long name, why not just return -EINVAL?
In this patch, if we pass a super long name, we're still trying to cut
it and request_module an invalid name, right?
Although '\0' is kept, but name is still invalid, right?


>                request_module("%s", elv);
>                spin_lock(&elv_list_lock);
> --
> 1.6.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>