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

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

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

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.

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.

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

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?

Thanks.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees