Re: [PATCH v2] net: ipv6: fix use-after-free Read in __xfrm6_tunnel_spi_lookup

[B K Karthik <bkkarthik@pesu.pes.edu>]

On Mon, Jul 27, 2020 at 1:37 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sat, Jul 25, 2020 at 11:12 PM B K Karthik <bkkarthik@pesu.pes.edu> wrote:
> >
> > On Sun, Jul 26, 2020 at 11:05 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Sat, Jul 25, 2020 at 8:09 PM B K Karthik <bkkarthik@pesu.pes.edu> wrote:
> > > > @@ -103,10 +103,10 @@ static int __xfrm6_tunnel_spi_check(struct net *net, u32 spi)
> > > >  {
> > > >         struct xfrm6_tunnel_net *xfrm6_tn = xfrm6_tunnel_pernet(net);
> > > >         struct xfrm6_tunnel_spi *x6spi;
> > > > -       int index = xfrm6_tunnel_spi_hash_byspi(spi);
> > > > +       int index = xfrm6_tunnel_spi_hash_byaddr((const xfrm_address_t *)spi);
> > > >
> > > >         hlist_for_each_entry(x6spi,
> > > > -                            &xfrm6_tn->spi_byspi[index],
> > > > +                            &xfrm6_tn->spi_byaddr[index],
> > > >                              list_byspi) {
> > > >                 if (x6spi->spi == spi)
> > >
> > > How did you convince yourself this is correct? This lookup is still
> > > using spi. :)
> >
> > I'm sorry, but my intention behind writing this patch was not to fix
> > the UAF, but to fix a slab-out-of-bound.
>
> Odd, your $subject is clearly UAF, so is the stack trace in your changelog.
> :)
>
>
> > If required, I can definitely change the subject line and resend the
> > patch, but I figured this was correct for
> > https://syzkaller.appspot.com/bug?id=058d05f470583ab2843b1d6785fa8d0658ef66ae
> > . since that particular report did not have a reproducer,
> > Dmitry Vyukov <dvyukov@google.com> suggested that I test this patch on
> > other reports for xfrm/spi .
>
> You have to change it to avoid misleading.

I will do that once somebody tells me this patch is reasonable to
avoid wasting people's time.
>
> >
> > Forgive me if this was the wrong way to send a patch for that
> > particular report, but I guessed since the reproducer did not trigger
> > the crash
> > for UAF, I would leave the subject line as 'fix UAF' :)
> >
> > xfrm6_spi_hash_by_hash seemed more convincing because I had to prevent
> > a slab-out-of-bounds because it uses ipv6_addr_hash.
> > It would be of great help if you could help me understand how this was
> > able to fix a UAF.
>
> Sure, you just avoid a pointer deref, which of course can fix the UAF,
> but I still don't think it is correct in any aspect.

I saw a function call being made to tomoyo_check_acl(). the next thing
happening is a kfree().
Also, spi_hash_byspi just returns spi % XFRM6_TUNNEL_SPI_BYSPI_HSIZE .

I'm a mentee, hence I would say my knowledge is very limited, please
let me know if I am making a horrible mistake somewhere,
but return (__force u32)(a->s6_addr32[0] ^ a->s6_addr32[1] ^
a->s6_addr32[2] ^ a->s6_addr32[3]); seems like a better because
as David S. Miller <davem@davemloft.net> said "It is doing a XOR on
all bits of an IPv6 address, it is doing more bit shifting which the
existing hash was ignoring" .

Please help me understand this better if I am going wrong.

>
> Even if it is a OOB, you still have to explain why it happened. Once
> again, I can't see how it could happen either.
>
> >
> > >
> > > More importantly, can you explain how UAF happens? Apparently
> > > the syzbot stack traces you quote make no sense at all. I also
> > > looked at other similar reports, none of them makes sense to me.
> >
> > Forgive me, but I do not understand what you mean by the stack traces
> > (this or other similar reports) "make no sense".
>
> Because the stack trace in your changelog clearly shows it is allocated
> in tomoyo_init_log(), which is a buffer in struct tomoyo_query, but
> none of xfrm paths uses it. Or do you see anything otherwise?

Aren't there indirect inet calls and netfilter hooks? I'm sorry I do
not see anything otherwise.
Please help me understand.

thanks,

karthik