| Message ID | 20170812120510.28750-6-phil@nwl.cc |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Sat, 12 Aug 2017 14:04:24 +0200 Phil Sutter <phil@nwl.cc> wrote: > can_state_names array contains at most CAN_STATE_MAX fields, so allowing > an index to it to be equal to that number is wrong. While here, also > make sure the array is indeed that big so nothing bad happens if > CAN_STATE_MAX ever increases. No more speculative bug fixes.
On Tue, Aug 15, 2017 at 08:10:49AM -0700, Stephen Hemminger wrote: > On Sat, 12 Aug 2017 14:04:24 +0200 > Phil Sutter <phil@nwl.cc> wrote: > > > can_state_names array contains at most CAN_STATE_MAX fields, so allowing > > an index to it to be equal to that number is wrong. While here, also > > make sure the array is indeed that big so nothing bad happens if > > CAN_STATE_MAX ever increases. > > No more speculative bug fixes. I don't think a bit of speculation regarding forwards-compatibility is a bad thing per se. In this case it is about the possibility for kernel code to add a new state to enum can_state. Older ip binaries will allow an index of CAN_STATE_MAX and therefore access data beyond end of can-state_names array. If you update linux headers but forget to add the new state to can_state_names array, the same will happen even if the sanity check for 'state' value is being fixed as by my patch. By specifying the size of can_state_names array upon definition, can_print_opt() will just print a null pointer which printf() can handle. Cheers, Phil
diff --git a/ip/iplink_can.c b/ip/iplink_can.c index 5df56b2bbbb3b..2954010fefa22 100644 --- a/ip/iplink_can.c +++ b/ip/iplink_can.c @@ -251,7 +251,7 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv, return 0; } -static const char *can_state_names[] = { +static const char *can_state_names[CAN_STATE_MAX] = { [CAN_STATE_ERROR_ACTIVE] = "ERROR-ACTIVE", [CAN_STATE_ERROR_WARNING] = "ERROR-WARNING", [CAN_STATE_ERROR_PASSIVE] = "ERROR-PASSIVE", @@ -275,7 +275,7 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) if (tb[IFLA_CAN_STATE]) { uint32_t state = rta_getattr_u32(tb[IFLA_CAN_STATE]); - fprintf(f, "state %s ", state <= CAN_STATE_MAX ? + fprintf(f, "state %s ", state < CAN_STATE_MAX ? can_state_names[state] : "UNKNOWN"); }
can_state_names array contains at most CAN_STATE_MAX fields, so allowing an index to it to be equal to that number is wrong. While here, also make sure the array is indeed that big so nothing bad happens if CAN_STATE_MAX ever increases. Signed-off-by: Phil Sutter <phil@nwl.cc> --- ip/iplink_can.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)