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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 24 Aug 2020 19:43:16 +0530
From:   Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
To:     Xie He <xie.he.0141@...il.com>
Cc:     Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>,
        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 12:12:01PM -0700, Xie He wrote:
> 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!
>
Sure, I had a look at it and since you are already working on fixing
this driver, don't think there is a need for a patch to fix the
particular race condition bug. This bug was found by the Linux driver
verification project and my work was to report it to the maintainers.

Thanks and Regards,
Madhuparna


> Thanks,
> Xie He