| Message ID | 1446209299-6250-1-git-send-email-marex@denx.de |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/30/2015 01:48 PM, Marek Vasut wrote: > The sizeof() is invoked on an incorrect variable, likely due to some > copy-paste error, and this might result in memory corruption. Fix this. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Wolfgang Grandegger <wg@grandegger.com> > Cc: Marc Kleine-Budde <mkl@pengutronix.de> > Cc: netdev@vger.kernel.org Applies to can and added stable on Cc. Thanks, Marc
On Friday, October 30, 2015 at 02:40:26 PM, Marc Kleine-Budde wrote: > On 10/30/2015 01:48 PM, Marek Vasut wrote: > > The sizeof() is invoked on an incorrect variable, likely due to some > > copy-paste error, and this might result in memory corruption. Fix this. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Wolfgang Grandegger <wg@grandegger.com> > > Cc: Marc Kleine-Budde <mkl@pengutronix.de> > > Cc: netdev@vger.kernel.org > > Applies to can and added stable on Cc. Are you absolutelly positive this doesn't break kernel ABI please ? I am a little worried there, since the size of can_clock and can_ctrlmode structures differ. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 03:01 PM, Marek Vasut wrote: > On Friday, October 30, 2015 at 02:40:26 PM, Marc Kleine-Budde wrote: >> On 10/30/2015 01:48 PM, Marek Vasut wrote: >>> The sizeof() is invoked on an incorrect variable, likely due to some >>> copy-paste error, and this might result in memory corruption. Fix this. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Wolfgang Grandegger <wg@grandegger.com> >>> Cc: Marc Kleine-Budde <mkl@pengutronix.de> >>> Cc: netdev@vger.kernel.org >> >> Applies to can and added stable on Cc. > > Are you absolutelly positive this doesn't break kernel ABI please ? > > I am a little worried there, since the size of can_clock and can_ctrlmode > structures differ. struct can_clock is a u32, see [1] struct can_ctrlmode is 2 x u32. in libsocketcan[2] it's accessed like this: > memcpy(res, > RTA_DATA(can_attr[IFLA_CAN_CLOCK]), > sizeof(struct can_clock)); I think it should be ok. Marc [1] http://lxr.free-electrons.com/source/include/uapi/linux/can/netlink.h#L61 [2] http://git.pengutronix.de/?p=tools/libsocketcan.git;a=blob;f=src/libsocketcan.c;h=c97a28cca18054c8e63326eeb5a866b79344ebe2;hb=4ea9ec7cf37a0c52f2c39a13887aaad11042ef5c#l453
On Friday, October 30, 2015 at 03:17:44 PM, Marc Kleine-Budde wrote: > On 10/30/2015 03:01 PM, Marek Vasut wrote: > > On Friday, October 30, 2015 at 02:40:26 PM, Marc Kleine-Budde wrote: > >> On 10/30/2015 01:48 PM, Marek Vasut wrote: > >>> The sizeof() is invoked on an incorrect variable, likely due to some > >>> copy-paste error, and this might result in memory corruption. Fix this. > >>> > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> Cc: Wolfgang Grandegger <wg@grandegger.com> > >>> Cc: Marc Kleine-Budde <mkl@pengutronix.de> > >>> Cc: netdev@vger.kernel.org > >> > >> Applies to can and added stable on Cc. > > > > Are you absolutelly positive this doesn't break kernel ABI please ? > > > > I am a little worried there, since the size of can_clock and can_ctrlmode > > structures differ. > > struct can_clock is a u32, see [1] > struct can_ctrlmode is 2 x u32. > > in libsocketcan[2] it's accessed like this: > > memcpy(res, > > > > RTA_DATA(can_attr[IFLA_CAN_CLOCK]), > > sizeof(struct can_clock)); > > I think it should be ok. In that case, yes, it's good. Hopefully, noone wrote his own thing. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 03:24 PM, Marek Vasut wrote: > On Friday, October 30, 2015 at 03:17:44 PM, Marc Kleine-Budde wrote: >> On 10/30/2015 03:01 PM, Marek Vasut wrote: >>> Are you absolutelly positive this doesn't break kernel ABI please ? >>> >>> I am a little worried there, since the size of can_clock and can_ctrlmode >>> structures differ. >> >> struct can_clock is a u32, see [1] >> struct can_ctrlmode is 2 x u32. >> >> in libsocketcan[2] it's accessed like this: >>> memcpy(res, >>> >>> RTA_DATA(can_attr[IFLA_CAN_CLOCK]), >>> sizeof(struct can_clock)); >> >> I think it should be ok. > > In that case, yes, it's good. Hopefully, noone wrote his own thing. > Fortunately ip from iproute2 does it similary: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/ip/iplink_can.c#n338 if (tb[IFLA_CAN_CLOCK]) { struct can_clock *clock = RTA_DATA(tb[IFLA_CAN_CLOCK]); fprintf(f, "\n clock %d", clock->freq); } As the clock is a read-only value kernel->userspace and nla_put creates its own small ID/length information each time we are REALLY LUCKY that this fix doesn't break the ABI in this case. When can_clock would have been greater then can_ctrlmode we really had a problem ... Thanks for caching this! Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, October 30, 2015 at 03:53:31 PM, Oliver Hartkopp wrote: > On 10/30/2015 03:24 PM, Marek Vasut wrote: > > On Friday, October 30, 2015 at 03:17:44 PM, Marc Kleine-Budde wrote: > >> On 10/30/2015 03:01 PM, Marek Vasut wrote: > >>> Are you absolutelly positive this doesn't break kernel ABI please ? > >>> > >>> I am a little worried there, since the size of can_clock and > >>> can_ctrlmode structures differ. > >> > >> struct can_clock is a u32, see [1] > >> struct can_ctrlmode is 2 x u32. > >> > >> in libsocketcan[2] it's accessed like this: > >>> memcpy(res, > >>> > >>> RTA_DATA(can_attr[IFLA_CAN_CLOCK]), > >>> sizeof(struct can_clock)); > >> > >> I think it should be ok. > > > > In that case, yes, it's good. Hopefully, noone wrote his own thing. > > Fortunately ip from iproute2 does it similary: > > https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/i > p/iplink_can.c#n338 > > > if (tb[IFLA_CAN_CLOCK]) { > struct can_clock *clock = RTA_DATA(tb[IFLA_CAN_CLOCK]); > > fprintf(f, "\n clock %d", clock->freq); > } > > As the clock is a read-only value kernel->userspace and nla_put creates its > own small ID/length information each time we are REALLY LUCKY that this fix > doesn't break the ABI in this case. > > When can_clock would have been greater then can_ctrlmode we really had a > problem ... > > Thanks for caching this! Yeah, I already had one leg in my asbestos trousers all right. Thanks for double-checking this! Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index aede704..141c2a4 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -915,7 +915,7 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev) nla_put(skb, IFLA_CAN_BITTIMING_CONST, sizeof(*priv->bittiming_const), priv->bittiming_const)) || - nla_put(skb, IFLA_CAN_CLOCK, sizeof(cm), &priv->clock) || + nla_put(skb, IFLA_CAN_CLOCK, sizeof(priv->clock), &priv->clock) || nla_put_u32(skb, IFLA_CAN_STATE, state) || nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) || nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
The sizeof() is invoked on an incorrect variable, likely due to some copy-paste error, and this might result in memory corruption. Fix this. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Wolfgang Grandegger <wg@grandegger.com> Cc: Marc Kleine-Budde <mkl@pengutronix.de> Cc: netdev@vger.kernel.org --- drivers/net/can/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) NOTE: I only compile-tested this.