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

On Mon, Jul 27, 2020 at 1:37 AM Cong Wang [off-list ref] wrote:

On Sat, Jul 25, 2020 at 11:12 PM B K Karthik [off-list ref] wrote:

quoted

On Sun, Jul 26, 2020 at 11:05 AM Cong Wang [off-list ref] wrote:

quoted

On Sat, Jul 25, 2020 at 8:09 PM B K Karthik [off-list ref] wrote:

quoted

@@ -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.
:)

quoted

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 [off-list ref] 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.

quoted

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 [off-list ref] 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.

quoted

quoted

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