On 1/4/19 10:13 AM, Oliver Hartkopp wrote: > Muyu Yu provided a POC where user root with CAP_NET_ADMIN can create a CAN > frame modification rule that makes the data length code a higher value than > the available CAN frame data size. In combination with a configured checksum > calculation where the result is stored relatively to the end of the data > (e.g. cgw_csum_xor_rel) the tail of the skb (e.g. frag_list pointer in > skb_shared_info) can be rewritten which finally can cause a system crash. > > Michael Kubecek suggested to drop frames that have a DLC exceeding the > available space after the modification process and provided a patch that can > handle CAN FD frames too. Within this patch we also limit the length for the > checksum calculations to the maximum of Classic CAN data length (8). > > CAN frames that are dropped by these additional checks are counted with the > CGW_DELETED counter which indicates misconfigurations in can-gw rules. > > This fixes CVE-2019-3701. > > Reported-by: Muyu Yu <[email protected]> > Reported-by: Marcus Meissner <[email protected]> > Suggested-by: Michal Kubecek <[email protected]> > Tested-by: Muyu Yu <[email protected]> > Tested-by: Oliver Hartkopp <[email protected]> > Signed-off-by: Oliver Hartkopp <[email protected]> > Cc: linux-stable <[email protected]> # >= v3.2 > --- > net/can/gw.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/net/can/gw.c b/net/can/gw.c > index faa3da88a127..180c389af5b1 100644 > --- a/net/can/gw.c > +++ b/net/can/gw.c > @@ -416,13 +416,31 @@ static void can_can_gw_rcv(struct sk_buff *skb, void > *data) > while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx]) > (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod); > > - /* check for checksum updates when the CAN frame has been modified */ > + /* Has the CAN frame been modified? */ > if (modidx) { > - if (gwj->mod.csumfunc.crc8) > - (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8); > + /* get available space for the processed CAN frame type */ > + int max_len = nskb->len - offsetof(struct can_frame, data); > > - if (gwj->mod.csumfunc.xor) > - (*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor); > + /* dlc may have changed, make sure it fits to the CAN frame */ > + if (cf->can_dlc > max_len) > + goto out_delete; > + > + /* check for checksum updates in classic CAN length only */ > + if (gwj->mod.csumfunc.crc8) { > + if (cf->can_dlc > 8) > + goto out_delete; > + else > + (*gwj->mod.csumfunc.crc8) > + (cf, &gwj->mod.csum.crc8);
What about removing the else, then we can keep this in one line.
> + }
> +
> + if (gwj->mod.csumfunc.xor) {
> + if (cf->can_dlc > 8)
> + goto out_delete;
> + else
> + (*gwj->mod.csumfunc.xor)
> + (cf, &gwj->mod.csum.xor);
same here
> + }
> }
>
> /* clear the skb timestamp if not configured the other way */
> @@ -434,6 +452,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void
> *data)
> gwj->dropped_frames++;
> else
> gwj->handled_frames++;
> +
> + return;
> +
> +out_delete:
> + /* delete frame due to misconfiguration */
> + gwj->deleted_frames++;
> + kfree_skb(nskb);
> + return;
> }
>
> static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
