netdev - Re: Regarding possible bug in net/wan/x25

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJht_EPrOuk3uweCNy06s0UQTBwkwCzjoS9fMfP8DMRAt8UV8w@mail.gmail.com>
Date:   Sun, 23 Aug 2020 12:12:01 -0700
From:   Xie He <xie.he.0141@...il.com>
To:     Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        andrianov <andrianov@...ras.ru>, netdev <netdev@...r.kernel.org>
Subject: Re: Regarding possible bug in net/wan/x25_asy.c

On Sun, Aug 23, 2020 at 8:28 AM Madhuparna Bhowmik
<madhuparnabhowmik10@...il.com> wrote:
>
> sl->xhead is modified in both x25_asy_change_mtu() and
> x25_asy_write_wakeup(). However, sl->lock is not held in
> x25_asy_write_wakeup(). So, I am not sure if it is indeed possible to
> have a race between these two functions. If it is possible that these
> two functions can execute in parallel then the lock should be held in
> x25_asy_write_wakeup() as well. Please let me know if this race is
> possible.

I think you are right. These two functions do race with each other.
There seems to be nothing preventing them from racing. We need to hold
the lock in x25_asy_write_wakeup to prevent it from racing with
x25_asy_change_mtu.

By the way, I think this driver has bigger problems. We can see that
these function pairs are not symmetric with one another in what they
do:
"x25_asy_alloc" and "x25_asy_free";
"x25_asy_open" and "x25_asy_close";
"x25_asy_open_tty" and "x25_asy_close_tty";
"x25_asy_netdev_ops.ndo_open" and "x25_asy_netdev_ops.ndo_stop".

This not only makes the code messy, but also makes the actual runtime
behavior buggy.

I'm planning to fix this with this change:
https://github.com/hyanggi/linux/commit/66387f229168014024117d50ade01092e3c9932c
Please take a look if you are interested. Thanks!

Thanks,
Xie He