Hello,
syzbot hit the following crash on upstream commit
f3afe530d644488a074291da04a69a296ab63046 (Tue Feb 27 22:02:39 2018 +0000)
Merge branch 'fixes-v4.16-rc4' of
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
So far this crash happened 3 times on upstream.
C reproducer is attached.
syzkaller reproducer is attached.
Raw console output is attached.
compiler: gcc (GCC) 7.1.1 20170620
.config is attached.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: [email protected]
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.
audit: type=1400 audit(1519800493.311:7): avc: denied { map } for
pid=4238 comm="syzkaller305740" path="/root/syzkaller305740266" dev="sda1"
ino=16481 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1
==================================================================
BUG: KASAN: use-after-free in __lock_acquire+0x3d4d/0x3e00
kernel/locking/lockdep.c:3310
Read of size 8 at addr ffff8801afa039c0 by task syzkaller305740/4238
CPU: 1 PID: 4238 Comm: syzkaller305740 Not tainted 4.16.0-rc3+ #332
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:17 [inline]
dump_stack+0x194/0x24d lib/dump_stack.c:53
print_address_description+0x73/0x250 mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report+0x23b/0x360 mm/kasan/report.c:412
__asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
__lock_acquire+0x3d4d/0x3e00 kernel/locking/lockdep.c:3310
lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:3920
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
remove_wait_queue+0x81/0x350 kernel/sched/wait.c:50
ep_remove_wait_queue fs/eventpoll.c:596 [inline]
ep_unregister_pollwait.isra.7+0x18c/0x590 fs/eventpoll.c:614
ep_free+0x13f/0x320 fs/eventpoll.c:832
ep_eventpoll_release+0x44/0x60 fs/eventpoll.c:864
__fput+0x327/0x7e0 fs/file_table.c:209
____fput+0x15/0x20 fs/file_table.c:243
task_work_run+0x199/0x270 kernel/task_work.c:113
exit_task_work include/linux/task_work.h:22 [inline]
do_exit+0x9bb/0x1ad0 kernel/exit.c:865
do_group_exit+0x149/0x400 kernel/exit.c:968
SYSC_exit_group kernel/exit.c:979 [inline]
SyS_exit_group+0x1d/0x20 kernel/exit.c:977
do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x42/0xb7
RIP: 0033:0x43e958
RSP: 002b:00007ffe63a11468 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043e958
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004be300 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 00000000004002c8 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006cc160 R14: 0000000000000000 R15: 0000000000000000
Allocated by task 4238:
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459 [inline]
kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:552
__do_kmalloc_node mm/slab.c:3669 [inline]
__kmalloc_node+0x47/0x70 mm/slab.c:3676
kmalloc_node include/linux/slab.h:554 [inline]
kvmalloc_node+0x99/0xd0 mm/util.c:419
kvmalloc include/linux/mm.h:541 [inline]
kvzalloc include/linux/mm.h:549 [inline]
alloc_netdev_mqs+0x16d/0xfb0 net/core/dev.c:8299
ppp_create_interface drivers/net/ppp/ppp_generic.c:3018 [inline]
ppp_unattached_ioctl drivers/net/ppp/ppp_generic.c:866 [inline]
ppp_ioctl+0x1715/0x2a50 drivers/net/ppp/ppp_generic.c:602
vfs_ioctl fs/ioctl.c:46 [inline]
do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
SYSC_ioctl fs/ioctl.c:701 [inline]
SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x42/0xb7
Freed by task 4238:
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459 [inline]
__kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:520
kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:527
__cache_free mm/slab.c:3485 [inline]
kfree+0xd9/0x260 mm/slab.c:3800
kvfree+0x36/0x60 mm/util.c:438
netdev_freemem+0x4c/0x60 net/core/dev.c:8253
netdev_release+0x10a/0x160 net/core/net-sysfs.c:1502
device_release+0x7c/0x210 drivers/base/core.c:814
kobject_cleanup lib/kobject.c:646 [inline]
kobject_release lib/kobject.c:675 [inline]
kref_put include/linux/kref.h:70 [inline]
kobject_put+0x14c/0x250 lib/kobject.c:692
put_device+0x20/0x30 drivers/base/core.c:1931
free_netdev+0x2f5/0x400 net/core/dev.c:8410
ppp_destroy_interface+0x2bc/0x390 drivers/net/ppp/ppp_generic.c:3100
ppp_release+0x12b/0x1a0 drivers/net/ppp/ppp_generic.c:415
ppp_ioctl+0x3b1/0x2a50 drivers/net/ppp/ppp_generic.c:628
vfs_ioctl fs/ioctl.c:46 [inline]
do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
SYSC_ioctl fs/ioctl.c:701 [inline]
SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x42/0xb7
The buggy address belongs to the object at ffff8801afa02e40
which belongs to the cache kmalloc-4096 of size 4096
The buggy address is located 2944 bytes inside of
4096-byte region [ffff8801afa02e40, ffff8801afa03e40)
The buggy address belongs to the page:
page:ffffea0006be8080 count:1 mapcount:0 mapping:ffff8801afa02e40 index:0x0
compound_mapcount: 0
flags: 0x2fffc0000008100(slab|head)
raw: 02fffc0000008100 ffff8801afa02e40 0000000000000000 0000000100000001
raw: ffffea0006bee220 ffffea0006bee7a0 ffff8801dac00dc0 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8801afa03880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8801afa03900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8801afa03980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8801afa03a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8801afa03a80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to [email protected].
syzbot will keep track of this bug report.
If you forgot to add the Reported-by tag, once the fix for this bug is
merged
into any tree, please reply to this email with:
#syz fix: exact-commit-title
If you want to test a patch for this bug, please reply with:
#syz test: git://repo/address.git branch
and provide the patch inline or as an attachment.
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line in the email body.
[+ppp list and maintainer]
On Wed, Feb 28, 2018 at 08:59:02AM -0800, syzbot wrote:
> Hello,
>
> syzbot hit the following crash on upstream commit
> f3afe530d644488a074291da04a69a296ab63046 (Tue Feb 27 22:02:39 2018 +0000)
> Merge branch 'fixes-v4.16-rc4' of
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> So far this crash happened 3 times on upstream.
> C reproducer is attached.
> syzkaller reproducer is attached.
> Raw console output is attached.
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: [email protected]
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> audit: type=1400 audit(1519800493.311:7): avc: denied { map } for
> pid=4238 comm="syzkaller305740" path="/root/syzkaller305740266" dev="sda1"
> ino=16481 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
> tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1
> ==================================================================
> BUG: KASAN: use-after-free in __lock_acquire+0x3d4d/0x3e00
> kernel/locking/lockdep.c:3310
> Read of size 8 at addr ffff8801afa039c0 by task syzkaller305740/4238
>
> CPU: 1 PID: 4238 Comm: syzkaller305740 Not tainted 4.16.0-rc3+ #332
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:17 [inline]
> dump_stack+0x194/0x24d lib/dump_stack.c:53
> print_address_description+0x73/0x250 mm/kasan/report.c:256
> kasan_report_error mm/kasan/report.c:354 [inline]
> kasan_report+0x23b/0x360 mm/kasan/report.c:412
> __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
> __lock_acquire+0x3d4d/0x3e00 kernel/locking/lockdep.c:3310
> lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:3920
> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
> remove_wait_queue+0x81/0x350 kernel/sched/wait.c:50
> ep_remove_wait_queue fs/eventpoll.c:596 [inline]
> ep_unregister_pollwait.isra.7+0x18c/0x590 fs/eventpoll.c:614
> ep_free+0x13f/0x320 fs/eventpoll.c:832
> ep_eventpoll_release+0x44/0x60 fs/eventpoll.c:864
> __fput+0x327/0x7e0 fs/file_table.c:209
> ____fput+0x15/0x20 fs/file_table.c:243
> task_work_run+0x199/0x270 kernel/task_work.c:113
> exit_task_work include/linux/task_work.h:22 [inline]
> do_exit+0x9bb/0x1ad0 kernel/exit.c:865
> do_group_exit+0x149/0x400 kernel/exit.c:968
> SYSC_exit_group kernel/exit.c:979 [inline]
> SyS_exit_group+0x1d/0x20 kernel/exit.c:977
> do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
> RIP: 0033:0x43e958
> RSP: 002b:00007ffe63a11468 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043e958
> RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
> RBP: 00000000004be300 R08: 00000000000000e7 R09: ffffffffffffffd0
> R10: 00000000004002c8 R11: 0000000000000246 R12: 0000000000000001
> R13: 00000000006cc160 R14: 0000000000000000 R15: 0000000000000000
>
> Allocated by task 4238:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:552
> __do_kmalloc_node mm/slab.c:3669 [inline]
> __kmalloc_node+0x47/0x70 mm/slab.c:3676
> kmalloc_node include/linux/slab.h:554 [inline]
> kvmalloc_node+0x99/0xd0 mm/util.c:419
> kvmalloc include/linux/mm.h:541 [inline]
> kvzalloc include/linux/mm.h:549 [inline]
> alloc_netdev_mqs+0x16d/0xfb0 net/core/dev.c:8299
> ppp_create_interface drivers/net/ppp/ppp_generic.c:3018 [inline]
> ppp_unattached_ioctl drivers/net/ppp/ppp_generic.c:866 [inline]
> ppp_ioctl+0x1715/0x2a50 drivers/net/ppp/ppp_generic.c:602
> vfs_ioctl fs/ioctl.c:46 [inline]
> do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
> SYSC_ioctl fs/ioctl.c:701 [inline]
> SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
> do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
>
> Freed by task 4238:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:520
> kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:527
> __cache_free mm/slab.c:3485 [inline]
> kfree+0xd9/0x260 mm/slab.c:3800
> kvfree+0x36/0x60 mm/util.c:438
> netdev_freemem+0x4c/0x60 net/core/dev.c:8253
> netdev_release+0x10a/0x160 net/core/net-sysfs.c:1502
> device_release+0x7c/0x210 drivers/base/core.c:814
> kobject_cleanup lib/kobject.c:646 [inline]
> kobject_release lib/kobject.c:675 [inline]
> kref_put include/linux/kref.h:70 [inline]
> kobject_put+0x14c/0x250 lib/kobject.c:692
> put_device+0x20/0x30 drivers/base/core.c:1931
> free_netdev+0x2f5/0x400 net/core/dev.c:8410
> ppp_destroy_interface+0x2bc/0x390 drivers/net/ppp/ppp_generic.c:3100
> ppp_release+0x12b/0x1a0 drivers/net/ppp/ppp_generic.c:415
> ppp_ioctl+0x3b1/0x2a50 drivers/net/ppp/ppp_generic.c:628
> vfs_ioctl fs/ioctl.c:46 [inline]
> do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
> SYSC_ioctl fs/ioctl.c:701 [inline]
> SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
> do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
>
> The buggy address belongs to the object at ffff8801afa02e40
> which belongs to the cache kmalloc-4096 of size 4096
> The buggy address is located 2944 bytes inside of
> 4096-byte region [ffff8801afa02e40, ffff8801afa03e40)
> The buggy address belongs to the page:
> page:ffffea0006be8080 count:1 mapcount:0 mapping:ffff8801afa02e40 index:0x0
> compound_mapcount: 0
> flags: 0x2fffc0000008100(slab|head)
> raw: 02fffc0000008100 ffff8801afa02e40 0000000000000000 0000000100000001
> raw: ffffea0006bee220 ffffea0006bee7a0 ffff8801dac00dc0 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8801afa03880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8801afa03900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ffff8801afa03980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff8801afa03a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8801afa03a80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>
> ---
> This bug is generated by a dumb bot. It may contain errors.
> See https://goo.gl/tpsmEJ for details.
> Direct all questions to [email protected].
>
> syzbot will keep track of this bug report.
> If you forgot to add the Reported-by tag, once the fix for this bug is
> merged
> into any tree, please reply to this email with:
> #syz fix: exact-commit-title
> If you want to test a patch for this bug, please reply with:
> #syz test: git://repo/address.git branch
> and provide the patch inline or as an attachment.
> To mark this as a duplicate of another syzbot report, please reply with:
> #syz dup: exact-subject-of-another-report
> If it's a one-off invalid bug report, please reply with:
> #syz invalid
> Note: if the crash happens again, it will cause creation of a new bug
> report.
> Note: all commands must start from beginning of the line in the email body.
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. 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. Any chance that PPPIOCDETACH can simply be removed,
given that it's apparently been "deprecated" for 16 years? Does anyone use it?
#include <fcntl.h>
#include <linux/ppp-ioctl.h>
#include <sys/epoll.h>
#include <sys/ioctl.h>
int main()
{
int pppfd, epfd, unit = 0;
struct epoll_event event = { 0 };
pppfd = open("/dev/ppp", O_RDONLY);
ioctl(pppfd, PPPIOCNEWUNIT, &unit);
epfd = epoll_create(0x2000);
epoll_ctl(epfd, EPOLL_CTL_ADD, pppfd, &event);
ioctl(pppfd, PPPIOCDETACH, &unit);
}
- Eric
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.
On Fri, May 18, 2018 at 06:02:23PM +0200, Guillaume Nault wrote:
> 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.
>
There is eventpoll_release_file(), but it's not exported to modules. It might
work to call it, but it seems like a hack.
> > 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?
Yes, though after looking into it more I *think* these additional races aren't
actually possible, due to the 'f_count < 2' check. These races could only
happen with a shared fd table, but in that case fdget() would increment f_count
for the duration of each operation, resulting in 'f_count >= 2' if both ioctl()
and something else is running on the same file concurrently.
Note that this also means PPPIOCDETACH doesn't work at all if called from a
multithreaded application...
>
> > 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.
IMO, if we can get away with removing it without any users noticing, that would
be much better than trying to fix it with a VFS-level hack, and probably missing
some cases. I'll send a patch to get things started...
- Eric
From: Eric Biggers <[email protected]>
The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
before f_count has reached 0, which is fundamentally a bad idea. It
does check 'f_count < 2', which excludes concurrent operations on the
file since they would only be possible with a shared fd table, in which
case each fdget() would take a file reference. However, it fails to
account for the fact that even with 'f_count == 1' the file can still be
linked into epoll instances. As reported by syzbot, this can trivially
be used to cause a use-after-free.
Yet, the only known user of PPPIOCDETACH is pppd versions older than
ppp-2.4.2, which was released almost 15 years ago (November 2003).
Also, PPPIOCDETACH apparently stopped working reliably at around the
same time, when the f_count check was added to the kernel, e.g. see
https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
check makes PPPIOCDETACH only work in single-threaded applications; it
always fails if called from a multithreaded application.
All pppd versions released in the last 15 years just close() the file
descriptor instead.
Therefore, instead of hacking around this bug by exporting epoll
internals to modules, and probably missing other related bugs, just
remove the PPPIOCDETACH ioctl and see if anyone actually notices.
Reported-by: [email protected]
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: [email protected]
Signed-off-by: Eric Biggers <[email protected]>
---
Documentation/networking/ppp_generic.txt | 6 -----
drivers/net/ppp/ppp_generic.c | 29 ------------------------
fs/compat_ioctl.c | 1 -
include/uapi/linux/ppp-ioctl.h | 1 -
4 files changed, 37 deletions(-)
diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt
index 091d20273dcb..61daf4b39600 100644
--- a/Documentation/networking/ppp_generic.txt
+++ b/Documentation/networking/ppp_generic.txt
@@ -300,12 +300,6 @@ unattached instance are:
The ioctl calls available on an instance of /dev/ppp attached to a
channel are:
-* PPPIOCDETACH detaches the instance from the channel. This ioctl is
- deprecated since the same effect can be achieved by closing the
- instance. In order to prevent possible races this ioctl will fail
- with an EINVAL error if more than one file descriptor refers to this
- instance (i.e. as a result of dup(), dup2() or fork()).
-
* PPPIOCCONNECT connects this channel to a PPP interface. The
argument should point to an int containing the interface unit
number. It will return an EINVAL error if the channel is already
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index dc7c7ec43202..dce8812fe802 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -603,35 +603,6 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
goto out;
}
- if (cmd == PPPIOCDETACH) {
- /*
- * We have to be careful here... if the file descriptor
- * has been dup'd, we could have another process in the
- * middle of a poll using the same file *, so we had
- * better not free the interface data structures -
- * instead we fail the ioctl. Even in this case, we
- * shut down the interface if we are the owner of it.
- * Actually, we should get rid of PPPIOCDETACH, userland
- * (i.e. pppd) could achieve the same effect by closing
- * this fd and reopening /dev/ppp.
- */
- err = -EINVAL;
- if (pf->kind == INTERFACE) {
- ppp = PF_TO_PPP(pf);
- rtnl_lock();
- if (file == ppp->owner)
- unregister_netdevice(ppp->dev);
- rtnl_unlock();
- }
- if (atomic_long_read(&file->f_count) < 2) {
- ppp_release(NULL, file);
- err = 0;
- } else
- pr_warn("PPPIOCDETACH file->f_count=%ld\n",
- atomic_long_read(&file->f_count));
- goto out;
- }
-
if (pf->kind == CHANNEL) {
struct channel *pch;
struct ppp_channel *chan;
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index ef80085ed564..8285b570d635 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -917,7 +917,6 @@ COMPATIBLE_IOCTL(PPPIOCSDEBUG)
/* PPPIOCGIDLE is translated */
COMPATIBLE_IOCTL(PPPIOCNEWUNIT)
COMPATIBLE_IOCTL(PPPIOCATTACH)
-COMPATIBLE_IOCTL(PPPIOCDETACH)
COMPATIBLE_IOCTL(PPPIOCSMRRU)
COMPATIBLE_IOCTL(PPPIOCCONNECT)
COMPATIBLE_IOCTL(PPPIOCDISCONN)
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index b19a9c249b15..d46caf217ea4 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -106,7 +106,6 @@ struct pppol2tp_ioc_stats {
#define PPPIOCGIDLE _IOR('t', 63, struct ppp_idle) /* get idle time */
#define PPPIOCNEWUNIT _IOWR('t', 62, int) /* create new ppp unit */
#define PPPIOCATTACH _IOW('t', 61, int) /* attach to ppp unit */
-#define PPPIOCDETACH _IOW('t', 60, int) /* detach from ppp unit/chan */
#define PPPIOCSMRRU _IOW('t', 59, int) /* set multilink MRU */
#define PPPIOCCONNECT _IOW('t', 58, int) /* connect channel to unit */
#define PPPIOCDISCONN _IO('t', 57) /* disconnect channel */
--
2.17.0
On Tue, May 22, 2018 at 08:29:58PM -0700, Eric Biggers wrote:
> On Fri, May 18, 2018 at 06:02:23PM +0200, Guillaume Nault wrote:
> > 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.
> >
>
> There is eventpoll_release_file(), but it's not exported to modules. It might
> work to call it, but it seems like a hack.
>
> > > 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?
>
> Yes, though after looking into it more I *think* these additional races aren't
> actually possible, due to the 'f_count < 2' check. These races could only
> happen with a shared fd table, but in that case fdget() would increment f_count
> for the duration of each operation, resulting in 'f_count >= 2' if both ioctl()
> and something else is running on the same file concurrently.
>
> Note that this also means PPPIOCDETACH doesn't work at all if called from a
> multithreaded application...
>
> >
> > > 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.
>
> IMO, if we can get away with removing it without any users noticing, that would
> be much better than trying to fix it with a VFS-level hack, and probably missing
> some cases. I'll send a patch to get things started...
>
Yes, I fully agree. That looks much safer, and given the track record
of this ioctl I very much doubt anyone would depend on it.
On Tue, May 22, 2018 at 08:59:52PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
> before f_count has reached 0, which is fundamentally a bad idea. It
> does check 'f_count < 2', which excludes concurrent operations on the
> file since they would only be possible with a shared fd table, in which
> case each fdget() would take a file reference. However, it fails to
> account for the fact that even with 'f_count == 1' the file can still be
> linked into epoll instances. As reported by syzbot, this can trivially
> be used to cause a use-after-free.
>
> Yet, the only known user of PPPIOCDETACH is pppd versions older than
> ppp-2.4.2, which was released almost 15 years ago (November 2003).
> Also, PPPIOCDETACH apparently stopped working reliably at around the
> same time, when the f_count check was added to the kernel, e.g. see
> https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
> check makes PPPIOCDETACH only work in single-threaded applications; it
> always fails if called from a multithreaded application.
>
> All pppd versions released in the last 15 years just close() the file
> descriptor instead.
>
> Therefore, instead of hacking around this bug by exporting epoll
> internals to modules, and probably missing other related bugs, just
> remove the PPPIOCDETACH ioctl and see if anyone actually notices.
>
> Reported-by: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
> ---
> Documentation/networking/ppp_generic.txt | 6 -----
> drivers/net/ppp/ppp_generic.c | 29 ------------------------
> fs/compat_ioctl.c | 1 -
> include/uapi/linux/ppp-ioctl.h | 1 -
> 4 files changed, 37 deletions(-)
>
> diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt
> index 091d20273dcb..61daf4b39600 100644
> --- a/Documentation/networking/ppp_generic.txt
> +++ b/Documentation/networking/ppp_generic.txt
> @@ -300,12 +300,6 @@ unattached instance are:
> The ioctl calls available on an instance of /dev/ppp attached to a
> channel are:
>
> -* PPPIOCDETACH detaches the instance from the channel. This ioctl is
> - deprecated since the same effect can be achieved by closing the
> - instance. In order to prevent possible races this ioctl will fail
> - with an EINVAL error if more than one file descriptor refers to this
> - instance (i.e. as a result of dup(), dup2() or fork()).
> -
> * PPPIOCCONNECT connects this channel to a PPP interface. The
> argument should point to an int containing the interface unit
> number. It will return an EINVAL error if the channel is already
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index dc7c7ec43202..dce8812fe802 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -603,35 +603,6 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> goto out;
> }
>
> - if (cmd == PPPIOCDETACH) {
> - /*
> - * We have to be careful here... if the file descriptor
> - * has been dup'd, we could have another process in the
> - * middle of a poll using the same file *, so we had
> - * better not free the interface data structures -
> - * instead we fail the ioctl. Even in this case, we
> - * shut down the interface if we are the owner of it.
> - * Actually, we should get rid of PPPIOCDETACH, userland
> - * (i.e. pppd) could achieve the same effect by closing
> - * this fd and reopening /dev/ppp.
> - */
> - err = -EINVAL;
> - if (pf->kind == INTERFACE) {
> - ppp = PF_TO_PPP(pf);
> - rtnl_lock();
> - if (file == ppp->owner)
> - unregister_netdevice(ppp->dev);
> - rtnl_unlock();
> - }
> - if (atomic_long_read(&file->f_count) < 2) {
> - ppp_release(NULL, file);
> - err = 0;
> - } else
> - pr_warn("PPPIOCDETACH file->f_count=%ld\n",
> - atomic_long_read(&file->f_count));
> - goto out;
> - }
> -
I'd rather add
+ if (cmd == PPPIOCDETACH) {
+ err = -EINVAL;
+ goto out;
+ }
Making PPPIOCDETACH unknown to ppp_generic means that the ioctl would
be handled by the underlying channel when pf->kind == CHANNEL (see the
chan->ops->ioctl() call further down). That shouldn't be a problem per
se, but even though PPPIOCDETACH is unsupported, I feel that it should
remain a ppp_generic thing. I don't really want its value to be reused
for other purposes in the future or have different behaviour depending
on the underlying channel.
Also PPPIOCDETACH can already fail with -EINVAL. Therefore, if ever
there really were programs out there using this call, they'd already
have to handle this case. Unconditionally returning -EINVAL would
further minimise possibilities for breakage.
From: Guillaume Nault <[email protected]>
Date: Wed, 23 May 2018 15:57:08 +0200
> I'd rather add
> + if (cmd == PPPIOCDETACH) {
> + err = -EINVAL;
> + goto out;
> + }
>
> Making PPPIOCDETACH unknown to ppp_generic means that the ioctl would
> be handled by the underlying channel when pf->kind == CHANNEL (see the
> chan->ops->ioctl() call further down). That shouldn't be a problem per
> se, but even though PPPIOCDETACH is unsupported, I feel that it should
> remain a ppp_generic thing. I don't really want its value to be reused
> for other purposes in the future or have different behaviour depending
> on the underlying channel.
>
> Also PPPIOCDETACH can already fail with -EINVAL. Therefore, if ever
> there really were programs out there using this call, they'd already
> have to handle this case. Unconditionally returning -EINVAL would
> further minimise possibilities for breakage.
I agree.
On Wed, May 23, 2018 at 11:56:36AM -0400, David Miller wrote:
> From: Guillaume Nault <[email protected]>
> Date: Wed, 23 May 2018 15:57:08 +0200
>
> > I'd rather add
> > + if (cmd == PPPIOCDETACH) {
> > + err = -EINVAL;
> > + goto out;
> > + }
> >
> > Making PPPIOCDETACH unknown to ppp_generic means that the ioctl would
> > be handled by the underlying channel when pf->kind == CHANNEL (see the
> > chan->ops->ioctl() call further down). That shouldn't be a problem per
> > se, but even though PPPIOCDETACH is unsupported, I feel that it should
> > remain a ppp_generic thing. I don't really want its value to be reused
> > for other purposes in the future or have different behaviour depending
> > on the underlying channel.
> >
> > Also PPPIOCDETACH can already fail with -EINVAL. Therefore, if ever
> > there really were programs out there using this call, they'd already
> > have to handle this case. Unconditionally returning -EINVAL would
> > further minimise possibilities for breakage.
>
> I agree.
Okay, I'll do that and leave the ioctl number reserved.
I will add a pr_warn_once() too.
Thanks,
- Eric
From: Eric Biggers <[email protected]>
The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
before f_count has reached 0, which is fundamentally a bad idea. It
does check 'f_count < 2', which excludes concurrent operations on the
file since they would only be possible with a shared fd table, in which
case each fdget() would take a file reference. However, it fails to
account for the fact that even with 'f_count == 1' the file can still be
linked into epoll instances. As reported by syzbot, this can trivially
be used to cause a use-after-free.
Yet, the only known user of PPPIOCDETACH is pppd versions older than
ppp-2.4.2, which was released almost 15 years ago (November 2003).
Also, PPPIOCDETACH apparently stopped working reliably at around the
same time, when the f_count check was added to the kernel, e.g. see
https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
check makes PPPIOCDETACH only work in single-threaded applications; it
always fails if called from a multithreaded application.
All pppd versions released in the last 15 years just close() the file
descriptor instead.
Therefore, instead of hacking around this bug by exporting epoll
internals to modules, and probably missing other related bugs, just
remove the PPPIOCDETACH ioctl and see if anyone actually notices. Leave
a stub in place that prints a one-time warning and returns EINVAL.
Reported-by: [email protected]
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: [email protected]
Signed-off-by: Eric Biggers <[email protected]>
---
v2: leave a stub in place, rather than removing the ioctl completely.
Documentation/networking/ppp_generic.txt | 6 ------
drivers/net/ppp/ppp_generic.c | 27 +++++-------------------
include/uapi/linux/ppp-ioctl.h | 2 +-
3 files changed, 6 insertions(+), 29 deletions(-)
diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt
index 091d20273dcb..61daf4b39600 100644
--- a/Documentation/networking/ppp_generic.txt
+++ b/Documentation/networking/ppp_generic.txt
@@ -300,12 +300,6 @@ unattached instance are:
The ioctl calls available on an instance of /dev/ppp attached to a
channel are:
-* PPPIOCDETACH detaches the instance from the channel. This ioctl is
- deprecated since the same effect can be achieved by closing the
- instance. In order to prevent possible races this ioctl will fail
- with an EINVAL error if more than one file descriptor refers to this
- instance (i.e. as a result of dup(), dup2() or fork()).
-
* PPPIOCCONNECT connects this channel to a PPP interface. The
argument should point to an int containing the interface unit
number. It will return an EINVAL error if the channel is already
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index dc7c7ec43202..02ad03a2fab7 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -605,30 +605,13 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (cmd == PPPIOCDETACH) {
/*
- * We have to be careful here... if the file descriptor
- * has been dup'd, we could have another process in the
- * middle of a poll using the same file *, so we had
- * better not free the interface data structures -
- * instead we fail the ioctl. Even in this case, we
- * shut down the interface if we are the owner of it.
- * Actually, we should get rid of PPPIOCDETACH, userland
- * (i.e. pppd) could achieve the same effect by closing
- * this fd and reopening /dev/ppp.
+ * PPPIOCDETACH is no longer supported as it was heavily broken,
+ * and is only known to have been used by pppd older than
+ * ppp-2.4.2 (released November 2003).
*/
+ pr_warn_once("%s (%d) used obsolete PPPIOCDETACH ioctl\n",
+ current->comm, current->pid);
err = -EINVAL;
- if (pf->kind == INTERFACE) {
- ppp = PF_TO_PPP(pf);
- rtnl_lock();
- if (file == ppp->owner)
- unregister_netdevice(ppp->dev);
- rtnl_unlock();
- }
- if (atomic_long_read(&file->f_count) < 2) {
- ppp_release(NULL, file);
- err = 0;
- } else
- pr_warn("PPPIOCDETACH file->f_count=%ld\n",
- atomic_long_read(&file->f_count));
goto out;
}
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index b19a9c249b15..784c2e3e572e 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -106,7 +106,7 @@ struct pppol2tp_ioc_stats {
#define PPPIOCGIDLE _IOR('t', 63, struct ppp_idle) /* get idle time */
#define PPPIOCNEWUNIT _IOWR('t', 62, int) /* create new ppp unit */
#define PPPIOCATTACH _IOW('t', 61, int) /* attach to ppp unit */
-#define PPPIOCDETACH _IOW('t', 60, int) /* detach from ppp unit/chan */
+#define PPPIOCDETACH _IOW('t', 60, int) /* obsolete, do not use */
#define PPPIOCSMRRU _IOW('t', 59, int) /* set multilink MRU */
#define PPPIOCCONNECT _IOW('t', 58, int) /* connect channel to unit */
#define PPPIOCDISCONN _IO('t', 57) /* disconnect channel */
--
2.17.0.441.gb46fe60e1d-goog
On Wed, May 23, 2018 at 02:37:38PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
> before f_count has reached 0, which is fundamentally a bad idea. It
> does check 'f_count < 2', which excludes concurrent operations on the
> file since they would only be possible with a shared fd table, in which
> case each fdget() would take a file reference. However, it fails to
> account for the fact that even with 'f_count == 1' the file can still be
> linked into epoll instances. As reported by syzbot, this can trivially
> be used to cause a use-after-free.
>
> Yet, the only known user of PPPIOCDETACH is pppd versions older than
> ppp-2.4.2, which was released almost 15 years ago (November 2003).
> Also, PPPIOCDETACH apparently stopped working reliably at around the
> same time, when the f_count check was added to the kernel, e.g. see
> https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
> check makes PPPIOCDETACH only work in single-threaded applications; it
> always fails if called from a multithreaded application.
>
> All pppd versions released in the last 15 years just close() the file
> descriptor instead.
>
> Therefore, instead of hacking around this bug by exporting epoll
> internals to modules, and probably missing other related bugs, just
> remove the PPPIOCDETACH ioctl and see if anyone actually notices. Leave
> a stub in place that prints a one-time warning and returns EINVAL.
>
> Reported-by: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
Acked-by: Paul Mackerras <[email protected]>
On Wed, May 23, 2018 at 02:37:38PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
> before f_count has reached 0, which is fundamentally a bad idea. It
> does check 'f_count < 2', which excludes concurrent operations on the
> file since they would only be possible with a shared fd table, in which
> case each fdget() would take a file reference. However, it fails to
> account for the fact that even with 'f_count == 1' the file can still be
> linked into epoll instances. As reported by syzbot, this can trivially
> be used to cause a use-after-free.
>
> Yet, the only known user of PPPIOCDETACH is pppd versions older than
> ppp-2.4.2, which was released almost 15 years ago (November 2003).
> Also, PPPIOCDETACH apparently stopped working reliably at around the
> same time, when the f_count check was added to the kernel, e.g. see
> https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
> check makes PPPIOCDETACH only work in single-threaded applications; it
> always fails if called from a multithreaded application.
>
> All pppd versions released in the last 15 years just close() the file
> descriptor instead.
>
> Therefore, instead of hacking around this bug by exporting epoll
> internals to modules, and probably missing other related bugs, just
> remove the PPPIOCDETACH ioctl and see if anyone actually notices. Leave
> a stub in place that prints a one-time warning and returns EINVAL.
>
> Reported-by: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
> ---
>
> v2: leave a stub in place, rather than removing the ioctl completely.
>
Thanks a lot for your help on this matter.
BTW, netdev has its own rules wrt. stable backports. You didn't need to
CC: stable@. David handles -stable submissions himself.
Using a 'PATCH net' subject prefix would have made it clear that this
patch was fixing some released code and should be considered for -stable
backport.
Reviewed-by: Guillaume Nault <[email protected]>
Tested-by: Guillaume Nault <[email protected]>
From: Eric Biggers <[email protected]>
Date: Wed, 23 May 2018 14:37:38 -0700
> From: Eric Biggers <[email protected]>
>
> The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
> before f_count has reached 0, which is fundamentally a bad idea. It
> does check 'f_count < 2', which excludes concurrent operations on the
> file since they would only be possible with a shared fd table, in which
> case each fdget() would take a file reference. However, it fails to
> account for the fact that even with 'f_count == 1' the file can still be
> linked into epoll instances. As reported by syzbot, this can trivially
> be used to cause a use-after-free.
>
> Yet, the only known user of PPPIOCDETACH is pppd versions older than
> ppp-2.4.2, which was released almost 15 years ago (November 2003).
> Also, PPPIOCDETACH apparently stopped working reliably at around the
> same time, when the f_count check was added to the kernel, e.g. see
> https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
> check makes PPPIOCDETACH only work in single-threaded applications; it
> always fails if called from a multithreaded application.
>
> All pppd versions released in the last 15 years just close() the file
> descriptor instead.
>
> Therefore, instead of hacking around this bug by exporting epoll
> internals to modules, and probably missing other related bugs, just
> remove the PPPIOCDETACH ioctl and see if anyone actually notices. Leave
> a stub in place that prints a one-time warning and returns EINVAL.
>
> Reported-by: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Biggers <[email protected]>
Applied and queued up for -stable.
> Eric Biggers <[email protected]> hat am 23. Mai 2018 um 23:37 geschrieben:
>
>
> From: Eric Biggers <[email protected]>
>
> The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
> before f_count has reached 0, which is fundamentally a bad idea. It
> does check 'f_count < 2', which excludes concurrent operations on the
> file since they would only be possible with a shared fd table, in which
> case each fdget() would take a file reference. However, it fails to
> account for the fact that even with 'f_count == 1' the file can still be
> linked into epoll instances. As reported by syzbot, this can trivially
> be used to cause a use-after-free.
>
> Yet, the only known user of PPPIOCDETACH is pppd versions older than
> ppp-2.4.2, which was released almost 15 years ago (November 2003).
> Also, PPPIOCDETACH apparently stopped working reliably at around the
> same time, when the f_count check was added to the kernel, e.g. see
> https://lkml.org/lkml/2002/12/31/83. Also, the current 'f_count < 2'
> check makes PPPIOCDETACH only work in single-threaded applications; it
> always fails if called from a multithreaded application.
>
> All pppd versions released in the last 15 years just close() the file
> descriptor instead.
>
> Therefore, instead of hacking around this bug by exporting epoll
> internals to modules, and probably missing other related bugs, just
> remove the PPPIOCDETACH ioctl and see if anyone actually notices. Leave
> a stub in place that prints a one-time warning and returns EINVAL.
>
> Reported-by: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
> ---
>
> v2: leave a stub in place, rather than removing the ioctl completely.
>
> Documentation/networking/ppp_generic.txt | 6 ------
> drivers/net/ppp/ppp_generic.c | 27 +++++-------------------
> include/uapi/linux/ppp-ioctl.h | 2 +-
> 3 files changed, 6 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/networking/ppp_generic.txt
> b/Documentation/networking/ppp_generic.txt
> index 091d20273dcb..61daf4b39600 100644
> --- a/Documentation/networking/ppp_generic.txt
> +++ b/Documentation/networking/ppp_generic.txt
> @@ -300,12 +300,6 @@ unattached instance are:
> The ioctl calls available on an instance of /dev/ppp attached to a
> channel are:
>
> -* PPPIOCDETACH detaches the instance from the channel. This ioctl is
> - deprecated since the same effect can be achieved by closing the
> - instance. In order to prevent possible races this ioctl will fail
> - with an EINVAL error if more than one file descriptor refers to this
> - instance (i.e. as a result of dup(), dup2() or fork()).
> -
> * PPPIOCCONNECT connects this channel to a PPP interface. The
> argument should point to an int containing the interface unit
> number. It will return an EINVAL error if the channel is already
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index dc7c7ec43202..02ad03a2fab7 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -605,30 +605,13 @@ static long ppp_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
>
> if (cmd == PPPIOCDETACH) {
> /*
> - * We have to be careful here... if the file descriptor
> - * has been dup'd, we could have another process in the
> - * middle of a poll using the same file *, so we had
> - * better not free the interface data structures -
> - * instead we fail the ioctl. Even in this case, we
> - * shut down the interface if we are the owner of it.
> - * Actually, we should get rid of PPPIOCDETACH, userland
> - * (i.e. pppd) could achieve the same effect by closing
> - * this fd and reopening /dev/ppp.
> + * PPPIOCDETACH is no longer supported as it was heavily broken,
> + * and is only known to have been used by pppd older than
> + * ppp-2.4.2 (released November 2003).
> */
> + pr_warn_once("%s (%d) used obsolete PPPIOCDETACH ioctl\n",
> + current->comm, current->pid);
> err = -EINVAL;
> - if (pf->kind == INTERFACE) {
> - ppp = PF_TO_PPP(pf);
> - rtnl_lock();
> - if (file == ppp->owner)
> - unregister_netdevice(ppp->dev);
> - rtnl_unlock();
> - }
> - if (atomic_long_read(&file->f_count) < 2) {
> - ppp_release(NULL, file);
> - err = 0;
> - } else
> - pr_warn("PPPIOCDETACH file->f_count=%ld\n",
> - atomic_long_read(&file->f_count));
> goto out;
> }
>
> diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
> index b19a9c249b15..784c2e3e572e 100644
> --- a/include/uapi/linux/ppp-ioctl.h
> +++ b/include/uapi/linux/ppp-ioctl.h
> @@ -106,7 +106,7 @@ struct pppol2tp_ioc_stats {
> #define PPPIOCGIDLE _IOR('t', 63, struct ppp_idle) /* get idle time */
> #define PPPIOCNEWUNIT _IOWR('t', 62, int) /* create new ppp unit */
> #define PPPIOCATTACH _IOW('t', 61, int) /* attach to ppp unit */
> -#define PPPIOCDETACH _IOW('t', 60, int) /* detach from ppp unit/chan */
> +#define PPPIOCDETACH _IOW('t', 60, int) /* obsolete, do not use */
It's a bit late but ...
i would suggest a stronger wording here. like /* removed 2003 */
for me "obsolete" sounds like "maybe it will be removed in a distant future"
just my 2 cents,
re,
wh
> #define PPPIOCSMRRU _IOW('t', 59, int) /* set multilink MRU */
> #define PPPIOCCONNECT _IOW('t', 58, int) /* connect channel to unit */
> #define PPPIOCDISCONN _IO('t', 57) /* disconnect channel */
> --
> 2.17.0.441.gb46fe60e1d-goog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ppp" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html