archive mirror
 help / color / mirror / Atom feed
From: Guillaume Nault <>
To: Eric Biggers <>
Cc:, Paul Mackerras <>,,,,
	syzbot <>,
Subject: Re: KASAN: use-after-free Read in remove_wait_queue (2)
Date: Fri, 18 May 2018 16:02:23 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20180514061155.GL677@sol.localdomain>

On Sun, May 13, 2018 at 11:11:55PM -0700, Eric Biggers wrote:
> [+ppp list and maintainer]
> This is a bug in ppp_generic.c; it still happens on Linus' tree and it's easily
> reproducible, see program below.  The bug is that the PPPIOCDETACH ioctl doesn't
> consider that the file can still be attached to epoll instances even when
> ->f_count = 1.

Right. What would it take to remove the file for the epoll instances?
Sorry for the naive question, but I'm not familiar with VFS and didn't
find a helper function we could call.

> Also, the reproducer doesn't test this but I think ppp_poll(),
> ppp_read(), and ppp_write() can all race with PPPIOCDETACH, causing
> use-after-frees as well.

I also believe so. ppp_release() resets ->private_data, and even though
functions like ppp_read() test ->private_data before executing, there's
no synchronisation mechanism to ensure that the update is visible
before the unit or channel is destroyed. Is that the kind of race you
had in mind?

> Any chance that PPPIOCDETACH can simply be removed,
> given that it's apparently been "deprecated" for 16 years?
> Does anyone use it?

The only users I'm aware of are pppd versions older than ppp-2.4.2
(released in November 2003). But even at that time, there were issues
with PPPIOCDETACH as pppd didn't seem to react properly when this call
failed. An Internet search on the "PPPIOCDETACH file->f_count=" kernel
log string, or on the "Couldn't release PPP unit: Invalid argument"
error message of pppd, returns several related bug reports.

Originally, PPPIOCDETACH never failed, but testing ->f_count was
later introduced to fix crashes when the file descriptor had been
duplicated. It seems that this was motivated by polling issues too.

Long story short, it looks like PPPIOCDETACH never has worked well
and we have at least two more bugs to fix. Given how it has proven
fragile, I wouldn't be surprised if there were even more lurking
around. I'd say that it's probably safer to drop it than to add more
workarounds and playing wack-a-mole with those bugs.

  reply	other threads:[~2018-05-18 16:02 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
2018-05-14  6:11 ` KASAN: use-after-free Read in remove_wait_queue (2) Eric Biggers
2018-05-18 16:02   ` Guillaume Nault [this message]
2018-05-23  3:29     ` Eric Biggers
2018-05-23  3:59       ` [PATCH] ppp: remove the PPPIOCDETACH ioctl Eric Biggers
2018-05-23 13:57         ` Guillaume Nault
2018-05-23 15:56           ` David Miller
2018-05-23 21:17             ` Eric Biggers
2018-05-23 21:37         ` [PATCH v2] " Eric Biggers
2018-05-23 23:04           ` Paul Mackerras
2018-05-24 14:04           ` Guillaume Nault
2018-05-25  2:55           ` David Miller
2018-06-06  9:01           ` Walter Harms
2018-05-23 13:26       ` KASAN: use-after-free Read in remove_wait_queue (2) Guillaume Nault

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).