| Message ID | 20220315203748.1892-1-socketcan@hartkopp.net (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] can: isotp: sanitize CAN ID checks in isotp_bind() | expand |
On Tue, 15 Mar 2022 21:37:48 +0100 Oliver Hartkopp wrote: > Syzbot created an environment that lead to a state machine status that > can not be reached with a compliant CAN ID address configuration. > The provided address information consisted of CAN ID 0x6000001 and 0xC28001 > which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. > > Sanitize the SFF/EFF CAN ID values before performing the address checks. > > Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> CC Marc, please make sure you CC maintainers.
On 16.03.22 02:51, Jakub Kicinski wrote: > On Tue, 15 Mar 2022 21:37:48 +0100 Oliver Hartkopp wrote: >> Syzbot created an environment that lead to a state machine status that >> can not be reached with a compliant CAN ID address configuration. >> The provided address information consisted of CAN ID 0x6000001 and 0xC28001 >> which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. >> >> Sanitize the SFF/EFF CAN ID values before performing the address checks. >> >> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") >> Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > > CC Marc, please make sure you CC maintainers. Oh, that would have been better! I'm maintaining the CAN network layer stuff together with Marc and there was no relevant stuff in can-next to be pulled in the next days. So I sent it directly to hit the merge window and had all of us in the reply to the syzbot report. Will CC Marc next time when posting to netdev only! Maybe I treated this patch more urgent than it needed to be handled ¯\_(ツ)_/¯ Thanks & best regards, Oliver
On 15.03.2022 18:51:34, Jakub Kicinski wrote: > On Tue, 15 Mar 2022 21:37:48 +0100 Oliver Hartkopp wrote: > > Syzbot created an environment that lead to a state machine status that > > can not be reached with a compliant CAN ID address configuration. > > The provided address information consisted of CAN ID 0x6000001 and 0xC28001 > > which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. > > > > Sanitize the SFF/EFF CAN ID values before performing the address checks. > > > > Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > > Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > > CC Marc, please make sure you CC maintainers. Thx. And make sure you have the linux-can ML on Cc :) regards, Marc
On 16.03.2022 08:35:58, Oliver Hartkopp wrote: > > > On 16.03.22 02:51, Jakub Kicinski wrote: > > On Tue, 15 Mar 2022 21:37:48 +0100 Oliver Hartkopp wrote: > > > Syzbot created an environment that lead to a state machine status that > > > can not be reached with a compliant CAN ID address configuration. > > > The provided address information consisted of CAN ID 0x6000001 and 0xC28001 > > > which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. > > > > > > Sanitize the SFF/EFF CAN ID values before performing the address checks. > > > > > > Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > > > Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com > > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > > > > CC Marc, please make sure you CC maintainers. > > Oh, that would have been better! I'm maintaining the CAN network layer stuff > together with Marc and there was no relevant stuff in can-next to be pulled > in the next days. So I sent it directly to hit the merge window and had all > of us in the reply to the syzbot report. > > Will CC Marc next time when posting to netdev only! > > Maybe I treated this patch more urgent than it needed to be handled > ¯\_(ツ)_/¯ Should this go into net/master with stable on Cc or to net-next? Marc
On 16.03.22 08:48, Marc Kleine-Budde wrote: > On 16.03.2022 08:35:58, Oliver Hartkopp wrote: >> >> >> On 16.03.22 02:51, Jakub Kicinski wrote: >>> On Tue, 15 Mar 2022 21:37:48 +0100 Oliver Hartkopp wrote: >>>> Syzbot created an environment that lead to a state machine status that >>>> can not be reached with a compliant CAN ID address configuration. >>>> The provided address information consisted of CAN ID 0x6000001 and 0xC28001 >>>> which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. >>>> >>>> Sanitize the SFF/EFF CAN ID values before performing the address checks. >>>> >>>> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") >>>> Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com >>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> >>> >>> CC Marc, please make sure you CC maintainers. >> >> Oh, that would have been better! I'm maintaining the CAN network layer stuff >> together with Marc and there was no relevant stuff in can-next to be pulled >> in the next days. So I sent it directly to hit the merge window and had all >> of us in the reply to the syzbot report. >> >> Will CC Marc next time when posting to netdev only! >> >> Maybe I treated this patch more urgent than it needed to be handled >> ¯\_(ツ)_/¯ > > Should this go into net/master with stable on Cc or to net-next? This patch is for net-next as it won't apply to net/master nor the stable trees due to the recent changes in net-next. As it contains a Fixes: tag I would send the patch for the stable trees when the patch hits Linus' tree and likely Greg would show up asking for it. I hope that's the most elegant process here?!? Best regards, Oliver
On 16.03.2022 08:53:54, Oliver Hartkopp wrote: > > Should this go into net/master with stable on Cc or to net-next? > > This patch is for net-next as it won't apply to net/master nor the > stable trees due to the recent changes in net-next. > > As it contains a Fixes: tag I would send the patch for the stable > trees when the patch hits Linus' tree and likely Greg would show up > asking for it. > > I hope that's the most elegant process here?!? Another option is to port the patch to net/master with the hope that back porting is easier. Then talk to Jakub and David about the merge conflict when net/master is merged to net-next/master. Marc
On 16.03.22 09:01, Marc Kleine-Budde wrote: > On 16.03.2022 08:53:54, Oliver Hartkopp wrote: >>> Should this go into net/master with stable on Cc or to net-next? >> >> This patch is for net-next as it won't apply to net/master nor the >> stable trees due to the recent changes in net-next. >> >> As it contains a Fixes: tag I would send the patch for the stable >> trees when the patch hits Linus' tree and likely Greg would show up >> asking for it. >> >> I hope that's the most elegant process here?!? > > Another option is to port the patch to net/master with the hope that > back porting is easier. I have a patch here for net/master that also properly applies down to linux-5.10.y If requested I could post it instantly. > Then talk to Jakub and David about the merge > conflict when net/master is merged to net-next/master. Yes. There will be a merge conflict. Therefore I thought bringing that patch for the released 5.17 and the stable trees later would be less effort for Jakub and David. Best regards. Oliver
Hi Marc, I sent a patch set with three patches on the CAN ML. Maybe it is just better when you pick them up for a regular process and send a pull request to Jakub and Dave. I still suggest to get these changes in via net-next and send the "sanitize CAN ID" backport for the 'older' kernels, when net-next hits Linus' tree. Best regards, Oliver On 16.03.22 09:42, Oliver Hartkopp wrote: > > > On 16.03.22 09:01, Marc Kleine-Budde wrote: >> On 16.03.2022 08:53:54, Oliver Hartkopp wrote: >>>> Should this go into net/master with stable on Cc or to net-next? >>> >>> This patch is for net-next as it won't apply to net/master nor the >>> stable trees due to the recent changes in net-next. >>> >>> As it contains a Fixes: tag I would send the patch for the stable >>> trees when the patch hits Linus' tree and likely Greg would show up >>> asking for it. >>> >>> I hope that's the most elegant process here?!? >> >> Another option is to port the patch to net/master with the hope that >> back porting is easier. > > I have a patch here for net/master that also properly applies down to > linux-5.10.y > If requested I could post it instantly. > >> Then talk to Jakub and David about the merge >> conflict when net/master is merged to net-next/master. > > Yes. There will be a merge conflict. Therefore I thought bringing that > patch for the released 5.17 and the stable trees later would be less > effort for Jakub and David. > > Best regards. > Oliver
On Wed, 16 Mar 2022 08:35:58 +0100 Oliver Hartkopp wrote: > On 16.03.22 02:51, Jakub Kicinski wrote: > > On Tue, 15 Mar 2022 21:37:48 +0100 Oliver Hartkopp wrote: > >> Syzbot created an environment that lead to a state machine status that > >> can not be reached with a compliant CAN ID address configuration. > >> The provided address information consisted of CAN ID 0x6000001 and 0xC28001 > >> which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. > >> > >> Sanitize the SFF/EFF CAN ID values before performing the address checks. > >> > >> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > >> Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com > >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > > > > CC Marc, please make sure you CC maintainers. > > Oh, that would have been better! I'm maintaining the CAN network layer > stuff together with Marc and there was no relevant stuff in can-next to > be pulled in the next days. So I sent it directly to hit the merge > window and had all of us in the reply to the syzbot report. > > Will CC Marc next time when posting to netdev only! > > Maybe I treated this patch more urgent than it needed to be handled > ¯\_(ツ)_/¯ Heh, I did think to myself "why is Oliver sending this directly" but wasn't confident enough to conclude that it's intentional :) Feel free to add any random info / context under the --- marker in a patch.
On Wed, 16 Mar 2022 09:42:14 +0100 Oliver Hartkopp wrote: > > Another option is to port the patch to net/master with the hope that > > back porting is easier. > > I have a patch here for net/master that also properly applies down to > linux-5.10.y > If requested I could post it instantly. > > > Then talk to Jakub and David about the merge > > conflict when net/master is merged to net-next/master. > > Yes. There will be a merge conflict. Therefore I thought bringing that > patch for the released 5.17 and the stable trees later would be less > effort for Jakub and David. No strong preference in this particular case, -rc9 is very unlikely so won't be much difference between it getting merge to net or net-next. If you don't have a preference either let's got with the usual process and send the patch to net, just share the conflict resolution and we can deal with it.
On 16.03.2022 17:49:35, Oliver Hartkopp wrote: > Hi Marc, > > I sent a patch set with three patches on the CAN ML. > > Maybe it is just better when you pick them up for a regular process and send > a pull request to Jakub and Dave. Done: https://lore.kernel.org/all/20220316204710.716341-1-mkl@pengutronix.de/ regards, Marc
diff --git a/net/can/isotp.c b/net/can/isotp.c index d4c0b4704987..1662103ce125 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1146,19 +1146,30 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) struct sock *sk = sock->sk; struct isotp_sock *so = isotp_sk(sk); struct net *net = sock_net(sk); int ifindex; struct net_device *dev; + canid_t tx_id, rx_id; int err = 0; int notify_enetdown = 0; int do_rx_reg = 1; if (len < ISOTP_MIN_NAMELEN) return -EINVAL; - if (addr->can_addr.tp.tx_id & (CAN_ERR_FLAG | CAN_RTR_FLAG)) - return -EADDRNOTAVAIL; + /* sanitize tx/rx CAN identifiers */ + tx_id = addr->can_addr.tp.tx_id; + if (tx_id & CAN_EFF_FLAG) + tx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); + else + tx_id &= CAN_SFF_MASK; + + rx_id = addr->can_addr.tp.rx_id; + if (rx_id & CAN_EFF_FLAG) + rx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); + else + rx_id &= CAN_SFF_MASK; if (!addr->can_ifindex) return -ENODEV; lock_sock(sk); @@ -1166,25 +1177,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) /* do not register frame reception for functional addressing */ if (so->opt.flags & CAN_ISOTP_SF_BROADCAST) do_rx_reg = 0; /* do not validate rx address for functional addressing */ - if (do_rx_reg) { - if (addr->can_addr.tp.rx_id == addr->can_addr.tp.tx_id) { - err = -EADDRNOTAVAIL; - goto out; - } - - if (addr->can_addr.tp.rx_id & (CAN_ERR_FLAG | CAN_RTR_FLAG)) { - err = -EADDRNOTAVAIL; - goto out; - } + if (do_rx_reg && rx_id == tx_id) { + err = -EADDRNOTAVAIL; + goto out; } if (so->bound && addr->can_ifindex == so->ifindex && - addr->can_addr.tp.rx_id == so->rxid && - addr->can_addr.tp.tx_id == so->txid) + rx_id == so->rxid && tx_id == so->txid) goto out; dev = dev_get_by_index(net, addr->can_ifindex); if (!dev) { err = -ENODEV; @@ -1204,20 +1207,18 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) notify_enetdown = 1; ifindex = dev->ifindex; if (do_rx_reg) { - can_rx_register(net, dev, addr->can_addr.tp.rx_id, - SINGLE_MASK(addr->can_addr.tp.rx_id), + can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id), isotp_rcv, sk, "isotp", sk); /* no consecutive frame echo skb in flight */ so->cfecho = 0; /* register for echo skb's */ - can_rx_register(net, dev, addr->can_addr.tp.tx_id, - SINGLE_MASK(addr->can_addr.tp.tx_id), + can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id), isotp_rcv_echo, sk, "isotpe", sk); } dev_put(dev); @@ -1237,12 +1238,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) } } /* switch to new settings */ so->ifindex = ifindex; - so->rxid = addr->can_addr.tp.rx_id; - so->txid = addr->can_addr.tp.tx_id; + so->rxid = rx_id; + so->txid = tx_id; so->bound = 1; out: release_sock(sk);
Syzbot created an environment that lead to a state machine status that can not be reached with a compliant CAN ID address configuration. The provided address information consisted of CAN ID 0x6000001 and 0xC28001 which both boil down to 11 bit CAN IDs 0x001 in sending and receiving. Sanitize the SFF/EFF CAN ID values before performing the address checks. Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") Reported-by: syzbot+2339c27f5c66c652843e@syzkaller.appspotmail.com Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> --- net/can/isotp.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)