Racy addr->valid in sctp_inet6addr_event() and sctp_copy_laddrs()

Racy addr->valid in sctp_inet6addr_event() and sctp_copy_laddrs()

[Sishuai Gong]

Hello,

We observed a data race over addr->valid between sctp_inet6addr_event() and 
sctp_copy_laddrs(). Under the following execution order, sctp_copy_laddrs()
might copy the addr that is no longer valid.

sctp_copy_laddrs()              sctp_inet6addr_event()
if (!addr->valid)
        continue;
                                		addr->valid = 0;
memcpy(&temp, &addr->a, sizeof(temp));

Thanks

Re: Racy addr->valid in sctp_inet6addr_event() and sctp_copy_laddrs()

[Xin Long]

On Tue, Aug 8, 2023 at 3:07 PM Sishuai Gong <sishuai.system@gmail.com> wrote:
>
> Hello,
>
> We observed a data race over addr->valid between sctp_inet6addr_event() and
> sctp_copy_laddrs(). Under the following execution order, sctp_copy_laddrs()
> might copy the addr that is no longer valid.
>
> sctp_copy_laddrs()              sctp_inet6addr_event()
> if (!addr->valid)
>         continue;
>                                                 addr->valid = 0;
> memcpy(&temp, &addr->a, sizeof(temp));
>
Yes, there's no lock protecting valid's read and write. But it doesn't
seem to matter.

Even if we put a lock there, after calling sctp_copy_laddrs() or
sctp_copy_local_addr_list() this addr can still be deleted in
NETDEV_DOWN event, which could happen anytime.

Checking addr->valid can only ensure the addr is available at that moment,
which doesn't look really helpful IMHO, considering an address can be
deleted, or the same address as the one already deleted can be added
anytime.

addr->valid and addr>rcu were introduced to fix an use-after-free. But it
seems to me that addr->rcu is enough for this purpose, we can actually
deleted addr->valid.