Message ID | 694fb074-53a3-af60-6561-57c022445a48@huawei.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: sit: fix UBSAN Undefined behaviour in check_6rd | expand |
From: linmiaohe <linmiaohe@huawei.com> Date: Mon, 11 Mar 2019 16:29:32 +0800 > @@ -778,8 +778,9 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst, > pbw0 = tunnel->ip6rd.prefixlen >> 5; > pbi0 = tunnel->ip6rd.prefixlen & 0x1f; > > - d = (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> > - tunnel->ip6rd.relay_prefixlen; > + d = tunnel->ip6rd.relay_prefixlen < 32 ? > + (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> > + tunnel->ip6rd.relay_prefixlen : 0; > I hate the fact that we have to guard against something which the rest of the code makes sure NEVER EVER happens. Every assignment of ->relay_prefixlen is guarded by a check against 32. I don't like this at all, and I have to put my foot down somehow. So I'm not applying this, sorry.
From: David Miller <davem@davemloft.net> Date: Mon, 11 Mar 2019 10:29:37 -0700 (PDT) > From: linmiaohe <linmiaohe@huawei.com> > Date: Mon, 11 Mar 2019 16:29:32 +0800 > >> @@ -778,8 +778,9 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst, >> pbw0 = tunnel->ip6rd.prefixlen >> 5; >> pbi0 = tunnel->ip6rd.prefixlen & 0x1f; >> >> - d = (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> >> - tunnel->ip6rd.relay_prefixlen; >> + d = tunnel->ip6rd.relay_prefixlen < 32 ? >> + (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> >> + tunnel->ip6rd.relay_prefixlen : 0; >> > > I hate the fact that we have to guard against something which the rest > of the code makes sure NEVER EVER happens. > > Every assignment of ->relay_prefixlen is guarded by a check against 32. Sorry, I now understand, it can equal 32. I'll apply this, thank you.
That's very nice of you. Thank you very much. On 2019/3/12 1:30, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Mon, 11 Mar 2019 10:29:37 -0700 (PDT) > >> From: linmiaohe <linmiaohe@huawei.com> >> Date: Mon, 11 Mar 2019 16:29:32 +0800 >> >>> @@ -778,8 +778,9 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst, >>> pbw0 = tunnel->ip6rd.prefixlen >> 5; >>> pbi0 = tunnel->ip6rd.prefixlen & 0x1f; >>> >>> - d = (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> >>> - tunnel->ip6rd.relay_prefixlen; >>> + d = tunnel->ip6rd.relay_prefixlen < 32 ? >>> + (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> >>> + tunnel->ip6rd.relay_prefixlen : 0; >>> >> >> I hate the fact that we have to guard against something which the rest >> of the code makes sure NEVER EVER happens. >> >> Every assignment of ->relay_prefixlen is guarded by a check against 32. > > Sorry, I now understand, it can equal 32. > > I'll apply this, thank you. > > . >
On 2019/3/12 1:30, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Mon, 11 Mar 2019 10:29:37 -0700 (PDT) > >> From: linmiaohe <linmiaohe@huawei.com> >> Date: Mon, 11 Mar 2019 16:29:32 +0800 >> >>> @@ -778,8 +778,9 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst, >>> pbw0 = tunnel->ip6rd.prefixlen >> 5; >>> pbi0 = tunnel->ip6rd.prefixlen & 0x1f; >>> >>> - d = (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> >>> - tunnel->ip6rd.relay_prefixlen; >>> + d = tunnel->ip6rd.relay_prefixlen < 32 ? >>> + (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> >>> + tunnel->ip6rd.relay_prefixlen : 0; >>> >> >> I hate the fact that we have to guard against something which the rest >> of the code makes sure NEVER EVER happens. >> >> Every assignment of ->relay_prefixlen is guarded by a check against 32. > > Sorry, I now understand, it can equal 32. > > I'll apply this, thank you. > > . > That's very nice of you. Thank you very much. I'am sorry for topping my reply in the previous email.
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index 09e440e8dfae..07e21a82ce4c 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -778,8 +778,9 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst, pbw0 = tunnel->ip6rd.prefixlen >> 5; pbi0 = tunnel->ip6rd.prefixlen & 0x1f; - d = (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> - tunnel->ip6rd.relay_prefixlen; + d = tunnel->ip6rd.relay_prefixlen < 32 ? + (ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >> + tunnel->ip6rd.relay_prefixlen : 0; pbi1 = pbi0 - tunnel->ip6rd.relay_prefixlen; if (pbi1 > 0)