[iproute,05/51] iplink_can: Prevent overstepping array bounds

Message ID 20170812120510.28750-6-phil@nwl.cc
State Changes Requested
Delegated to: stephen hemminger
Headers show

Commit Message

Phil Sutter Aug. 12, 2017, 12:04 p.m.
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(-)

Comments

Stephen Hemminger Aug. 15, 2017, 3:10 p.m. | #1
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.
Phil Sutter Aug. 15, 2017, 4:31 p.m. | #2
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

Patch

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");
 	}