: Check TCA_ACT_KIND payload size _before_ copying it
Subsystems (2)
PATCH 4/17 : Check TCA_ACT_KIND payload size _before_ copying it
Fix payload size checks like this one:
- sprintf(act_name, "%s", (char*)RTA_DATA(kind));
- if (RTA_PAYLOAD(kind) >= IFNAMSIZ) {
- printk("Action %s bad\n", (char*)RTA_DATA(kind))
# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/12/30 01:45:41+01:00 [email protected]
# [PKT_SCHED]: Check TCA_ACT_KIND payload size _before_ copying it
#
# Signed-off-by: Patrick McHardy <[email protected]>
#
# net/sched/act_api.c # 2004/12/30 01:45:35+01:00 [email protected] +6 -14 # [PKT_SCHED]: Check TCA_ACT_KIND payload size _before_ copying it # # Signed-off-by: Patrick McHardy <[email protected]> # diff -Nru a/net/sched/act_api.c b/net/sched/act_api.c --- a/net/sched/act_api.c 2004-12-30 04:01:10 +01:00 +++ b/net/sched/act_api.c 2004-12-30 04:01:10 +01:00 @@ -288,11 +288,9 @@ goto err_out; kind = tb[TCA_ACT_KIND-1]; if (kind != NULL) { - sprintf(act_name, "%s", (char*)RTA_DATA(kind)); - if (RTA_PAYLOAD(kind) >= IFNAMSIZ) { - printk("Action %s bad\n", (char*)RTA_DATA(kind)); + if (RTA_PAYLOAD(kind) >= IFNAMSIZ) goto err_out; - } + sprintf(act_name, "%s", (char*)RTA_DATA(kind)); } else { printk("Action bad kind\n"); goto err_out; @@ -503,12 +501,9 @@ goto err_out; kind = tb[TCA_ACT_KIND-1]; if (kind != NULL) { - sprintf(act_name, "%s", (char*)RTA_DATA(kind)); - if (RTA_PAYLOAD(kind) >= IFNAMSIZ) { - printk("tcf_action_get_1: action %s bad\n", - (char*)RTA_DATA(kind)); + if (RTA_PAYLOAD(kind) >= IFNAMSIZ) goto err_out; - } + sprintf(act_name, "%s", (char*)RTA_DATA(kind)); } else { printk("tcf_action_get_1: action bad kind\n"); goto err_out; @@ -567,12 +562,9 @@ struct tc_action_ops *a_o = NULL;
if (kind != NULL) {
- sprintf(act_name, "%s", (char*)RTA_DATA(kind));
- if (RTA_PAYLOAD(kind) >= IFNAMSIZ) {
- printk("get_ao: action %s bad\n",
- (char*)RTA_DATA(kind));
+ if (RTA_PAYLOAD(kind) >= IFNAMSIZ)
return NULL;
- }
+ sprintf(act_name, "%s", (char*)RTA_DATA(kind));
} else {
printk("get_ao: action bad kind\n"); return NULL;
* Patrick McHardy <[email protected]> 2004-12-30 04:39
Review
Quoted 4 lines
(quoted diff) +1
-3- sprintf(act_name, "%s", (char*)RTA_DATA(kind)); - if (RTA_PAYLOAD(kind) >= IFNAMSIZ) { - printk("Action %s bad\n", (char*)RTA_DATA(kind)); + if (RTA_PAYLOAD(kind) >= IFNAMSIZ)
The check should be RTA_PAYLOAD(kind) > IFNAMSIZ, == is ok if the terminating NUL is provided.
Review
Quoted 4 lines
(quoted diff) +1
-1goto err_out; - } + sprintf(act_name, "%s", (char*)RTA_DATA(kind)); } else {
This will cause horrible crashes if no NUL is provided to terminate the name.
So I think this should be:
if (RTA_PAYLOAD(kind) > IFNAMSIZ)
goto err_out;
memset(act_name, ...);memcpy(act_name, RTA_DATA(kind), RTA_PAYLOAD(kind)); act_name[IFNAMSIZ - 1] = '\0';
The memset is required to ensure 0 termination if kind is not and shorter than IFNAMSIZ. memcpy instead of str* to avoid using any form of str(n)len on a possibly not terminated string and setting IFNAMSIZ - 1 to NUL to ensure proper handling of a IFNAMSIZ long not terminated string.
I know it's unlikely but this might just save us some troubles later.
Quoted 10 lines · Thomas Graf
* Patrick McHardy <[email protected]> 2004-12-30 04:39
Review
Quoted 4 lines
(quoted diff) +1
-3- sprintf(act_name, "%s", (char*)RTA_DATA(kind)); - if (RTA_PAYLOAD(kind) >= IFNAMSIZ) { - printk("Action %s bad\n", (char*)RTA_DATA(kind)); + if (RTA_PAYLOAD(kind) >= IFNAMSIZ)The check should be
RTA_PAYLOAD(kind)> IFNAMSIZ, == is ok if the terminating NUL is provided.
Thanks.
Quoted 19 lines
Quoted 6 lines
goto err_out;(quoted diff) +1
-1- } + sprintf(act_name, "%s", (char*)RTA_DATA(kind));} else {This will cause horrible crashes if no NUL is provided to terminate the name.
So I think this should be:
if (
RTA_PAYLOAD(kind)> IFNAMSIZ)goto err_out;
memset(act_name, ...);memcpy(act_name, RTA_DATA(kind), RTA_PAYLOAD(kind)); act_name[IFNAMSIZ - 1] = '\0';
Quoted 7 lines
The memset is required to ensure 0 termination if kind is not and shorter than IFNAMSIZ. memcpy instead of str* to avoid using any form of str(n)len on a possibly not terminated string and setting IFNAMSIZ - 1 to NUL to ensure proper handling of a IFNAMSIZ long not terminated string.
I know it's unlikely but this might just save us some troubles later.
Agreed. I saved this change for later because there are more places in net/sched that need to be fixed. I guess I'll just add a rtattr_strncpy function.
Regards
Patrick