* [PATCH v6] block: Fix use-after-free in blkdev_get()
@ 2020-06-16 3:40 Jason Yan
2020-06-16 10:20 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Jason Yan @ 2020-06-16 3:40 UTC (permalink / raw)
To: axboe, linux-block, linux-fsdevel
Cc: Jason Yan, Christoph Hellwig, Ming Lei, Jan Kara, Dan Carpenter,
Hulk Robot, Sedat Dilek
In blkdev_get() we call __blkdev_get() to do some internal jobs and if
there is some errors in __blkdev_get(), the bdput() is called which
means we have released the refcount of the bdev (actually the refcount of
the bdev inode). This means we cannot access bdev after that point. But
acctually bdev is still accessed in blkdev_get() after calling
__blkdev_get(). This results in use-after-free if the refcount is the
last one we released in __blkdev_get(). Let's take a look at the
following scenerio:
CPU0 CPU1 CPU2
blkdev_open blkdev_open Remove disk
bd_acquire
blkdev_get
__blkdev_get del_gendisk
bdev_unhash_inode
bd_acquire bdev_get_gendisk
bd_forget failed because of unhashed
bdput
bdput (the last one)
bdev_evict_inode
access bdev => use after free
[ 459.350216] BUG: KASAN: use-after-free in __lock_acquire+0x24c1/0x31b0
[ 459.351190] Read of size 8 at addr ffff88806c815a80 by task syz-executor.0/20132
[ 459.352347]
[ 459.352594] CPU: 0 PID: 20132 Comm: syz-executor.0 Not tainted 4.19.90 #2
[ 459.353628] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 459.354947] Call Trace:
[ 459.355337] dump_stack+0x111/0x19e
[ 459.355879] ? __lock_acquire+0x24c1/0x31b0
[ 459.356523] print_address_description+0x60/0x223
[ 459.357248] ? __lock_acquire+0x24c1/0x31b0
[ 459.357887] kasan_report.cold+0xae/0x2d8
[ 459.358503] __lock_acquire+0x24c1/0x31b0
[ 459.359120] ? _raw_spin_unlock_irq+0x24/0x40
[ 459.359784] ? lockdep_hardirqs_on+0x37b/0x580
[ 459.360465] ? _raw_spin_unlock_irq+0x24/0x40
[ 459.361123] ? finish_task_switch+0x125/0x600
[ 459.361812] ? finish_task_switch+0xee/0x600
[ 459.362471] ? mark_held_locks+0xf0/0xf0
[ 459.363108] ? __schedule+0x96f/0x21d0
[ 459.363716] lock_acquire+0x111/0x320
[ 459.364285] ? blkdev_get+0xce/0xbe0
[ 459.364846] ? blkdev_get+0xce/0xbe0
[ 459.365390] __mutex_lock+0xf9/0x12a0
[ 459.365948] ? blkdev_get+0xce/0xbe0
[ 459.366493] ? bdev_evict_inode+0x1f0/0x1f0
[ 459.367130] ? blkdev_get+0xce/0xbe0
[ 459.367678] ? destroy_inode+0xbc/0x110
[ 459.368261] ? mutex_trylock+0x1a0/0x1a0
[ 459.368867] ? __blkdev_get+0x3e6/0x1280
[ 459.369463] ? bdev_disk_changed+0x1d0/0x1d0
[ 459.370114] ? blkdev_get+0xce/0xbe0
[ 459.370656] blkdev_get+0xce/0xbe0
[ 459.371178] ? find_held_lock+0x2c/0x110
[ 459.371774] ? __blkdev_get+0x1280/0x1280
[ 459.372383] ? lock_downgrade+0x680/0x680
[ 459.373002] ? lock_acquire+0x111/0x320
[ 459.373587] ? bd_acquire+0x21/0x2c0
[ 459.374134] ? do_raw_spin_unlock+0x4f/0x250
[ 459.374780] blkdev_open+0x202/0x290
[ 459.375325] do_dentry_open+0x49e/0x1050
[ 459.375924] ? blkdev_get_by_dev+0x70/0x70
[ 459.376543] ? __x64_sys_fchdir+0x1f0/0x1f0
[ 459.377192] ? inode_permission+0xbe/0x3a0
[ 459.377818] path_openat+0x148c/0x3f50
[ 459.378392] ? kmem_cache_alloc+0xd5/0x280
[ 459.379016] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 459.379802] ? path_lookupat.isra.0+0x900/0x900
[ 459.380489] ? __lock_is_held+0xad/0x140
[ 459.381093] do_filp_open+0x1a1/0x280
[ 459.381654] ? may_open_dev+0xf0/0xf0
[ 459.382214] ? find_held_lock+0x2c/0x110
[ 459.382816] ? lock_downgrade+0x680/0x680
[ 459.383425] ? __lock_is_held+0xad/0x140
[ 459.384024] ? do_raw_spin_unlock+0x4f/0x250
[ 459.384668] ? _raw_spin_unlock+0x1f/0x30
[ 459.385280] ? __alloc_fd+0x448/0x560
[ 459.385841] do_sys_open+0x3c3/0x500
[ 459.386386] ? filp_open+0x70/0x70
[ 459.386911] ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 459.387610] ? trace_hardirqs_off_caller+0x55/0x1c0
[ 459.388342] ? do_syscall_64+0x1a/0x520
[ 459.388930] do_syscall_64+0xc3/0x520
[ 459.389490] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 459.390248] RIP: 0033:0x416211
[ 459.390720] Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83
04 19 00 00 c3 48 83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f
05 <48> 8b 3c 24 48 89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d
01
[ 459.393483] RSP: 002b:00007fe45dfe9a60 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
[ 459.394610] RAX: ffffffffffffffda RBX: 00007fe45dfea6d4 RCX: 0000000000416211
[ 459.395678] RDX: 00007fe45dfe9b0a RSI: 0000000000000002 RDI: 00007fe45dfe9b00
[ 459.396758] RBP: 000000000076bf20 R08: 0000000000000000 R09: 000000000000000a
[ 459.397930] R10: 0000000000000075 R11: 0000000000000293 R12: 00000000ffffffff
[ 459.399022] R13: 0000000000000bd9 R14: 00000000004cdb80 R15: 000000000076bf2c
[ 459.400168]
[ 459.400430] Allocated by task 20132:
[ 459.401038] kasan_kmalloc+0xbf/0xe0
[ 459.401652] kmem_cache_alloc+0xd5/0x280
[ 459.402330] bdev_alloc_inode+0x18/0x40
[ 459.402970] alloc_inode+0x5f/0x180
[ 459.403510] iget5_locked+0x57/0xd0
[ 459.404095] bdget+0x94/0x4e0
[ 459.404607] bd_acquire+0xfa/0x2c0
[ 459.405113] blkdev_open+0x110/0x290
[ 459.405702] do_dentry_open+0x49e/0x1050
[ 459.406340] path_openat+0x148c/0x3f50
[ 459.406926] do_filp_open+0x1a1/0x280
[ 459.407471] do_sys_open+0x3c3/0x500
[ 459.408010] do_syscall_64+0xc3/0x520
[ 459.408572] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 459.409415]
[ 459.409679] Freed by task 1262:
[ 459.410212] __kasan_slab_free+0x129/0x170
[ 459.410919] kmem_cache_free+0xb2/0x2a0
[ 459.411564] rcu_process_callbacks+0xbb2/0x2320
[ 459.412318] __do_softirq+0x225/0x8ac
Fix this by delaying bdput() to the end of blkdev_get() which means we
have finished accessing bdev.
Fixes: e525fd89d380 ("block: make blkdev_get/put() handle exclusive access")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
---
v6: Add Tested-by tag from Sedat and cc Dan.
v5: Add fixes tag and Reviewed-by tag from Christoph.
v4: Remove uneeded braces and add Reviewed-by tag from Jan Kara.
v3: Add bdput() when __blkdev_get() calling itself failed.
v2: Add Reported-by tag and cc linux-block mailing list
fs/block_dev.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 47860e589388..08c87db3a92b 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1565,10 +1565,8 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
*/
if (!for_part) {
ret = devcgroup_inode_permission(bdev->bd_inode, perm);
- if (ret != 0) {
- bdput(bdev);
+ if (ret != 0)
return ret;
- }
}
restart:
@@ -1637,8 +1635,10 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
goto out_clear;
BUG_ON(for_part);
ret = __blkdev_get(whole, mode, 1);
- if (ret)
+ if (ret) {
+ bdput(whole);
goto out_clear;
+ }
bdev->bd_contains = whole;
bdev->bd_part = disk_get_part(disk, partno);
if (!(disk->flags & GENHD_FL_UP) ||
@@ -1688,7 +1688,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
disk_unblock_events(disk);
put_disk_and_module(disk);
out:
- bdput(bdev);
return ret;
}
@@ -1755,6 +1754,9 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
bdput(whole);
}
+ if (res)
+ bdput(bdev);
+
return res;
}
EXPORT_SYMBOL(blkdev_get);
--
2.25.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v6] block: Fix use-after-free in blkdev_get()
2020-06-16 3:40 [PATCH v6] block: Fix use-after-free in blkdev_get() Jason Yan
@ 2020-06-16 10:20 ` Dan Carpenter
2020-06-16 11:24 ` Jason Yan
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-06-16 10:20 UTC (permalink / raw)
To: Jason Yan
Cc: axboe, linux-block, linux-fsdevel, Christoph Hellwig, Ming Lei,
Jan Kara, Hulk Robot, Sedat Dilek
On Tue, Jun 16, 2020 at 11:40:02AM +0800, Jason Yan wrote:
>
> Fixes: e525fd89d380 ("block: make blkdev_get/put() handle exclusive access")
I still don't understand how this is the correct fixes tag... :/
git show e525fd89d380:fs/block_dev.c | cat -n
1208 int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
1209 {
1210 struct block_device *whole = NULL;
1211 int res;
1212
1213 WARN_ON_ONCE((mode & FMODE_EXCL) && !holder);
1214
1215 if ((mode & FMODE_EXCL) && holder) {
1216 whole = bd_start_claiming(bdev, holder);
1217 if (IS_ERR(whole)) {
1218 bdput(bdev);
1219 return PTR_ERR(whole);
1220 }
1221 }
1222
1223 res = __blkdev_get(bdev, mode, 0);
1224
1225 if (whole) {
1226 if (res == 0)
^^^^^^^^
1227 bd_finish_claiming(bdev, whole, holder);
1228 else
1229 bd_abort_claiming(whole, holder);
^^^^^^^^^^^^^
If __blkdev_get() then this doesn't dereference "bdev" so it's not a
use after free bug.
1230 }
1231
1232 return res;
1233 }
So far as I can see the Fixes tag should be what I said earlier.
Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")
Otherwise the patch looks good to me.
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6] block: Fix use-after-free in blkdev_get()
2020-06-16 10:20 ` Dan Carpenter
@ 2020-06-16 11:24 ` Jason Yan
2020-06-16 11:49 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Jason Yan @ 2020-06-16 11:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: axboe, linux-block, linux-fsdevel, Christoph Hellwig, Ming Lei,
Jan Kara, Hulk Robot, Sedat Dilek
Hi Dan，
在 2020/6/16 18:20, Dan Carpenter 写道:
> On Tue, Jun 16, 2020 at 11:40:02AM +0800, Jason Yan wrote:
>>
>> Fixes: e525fd89d380 ("block: make blkdev_get/put() handle exclusive access")
>
> I still don't understand how this is the correct fixes tag... :/
>
> git show e525fd89d380:fs/block_dev.c | cat -n
> 1208 int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
> 1209 {
> 1210 struct block_device *whole = NULL;
> 1211 int res;
> 1212
> 1213 WARN_ON_ONCE((mode & FMODE_EXCL) && !holder);
> 1214
> 1215 if ((mode & FMODE_EXCL) && holder) {
> 1216 whole = bd_start_claiming(bdev, holder);
> 1217 if (IS_ERR(whole)) {
> 1218 bdput(bdev);
> 1219 return PTR_ERR(whole);
> 1220 }
> 1221 }
> 1222
> 1223 res = __blkdev_get(bdev, mode, 0);
> 1224
> 1225 if (whole) {
> 1226 if (res == 0)
> ^^^^^^^^
>
> 1227 bd_finish_claiming(bdev, whole, holder);
> 1228 else
> 1229 bd_abort_claiming(whole, holder);
> ^^^^^^^^^^^^^
> If __blkdev_get() then this doesn't dereference "bdev" so it's not a
> use after free bug.
>
> 1230 }
> 1231
> 1232 return res;
> 1233 }
>
> So far as I can see the Fixes tag should be what I said earlier.
>
> Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")
>
I tried kernel before this commit and can still reproduce this issue.
After some digging, at last I found this one:
77ea887e433a "implement in-kernel gendisk events handling"
@@ -1158,9 +1159,10 @@ int blkdev_get(struct block_device *bdev, fmode_t
mode, void *holder)
if (whole) {
/* finish claiming */
+ mutex_lock(&bdev->bd_mutex);
spin_lock(&bdev_lock);
- if (res == 0) {
+ if (!res) {
BUG_ON(!bd_may_claim(bdev, whole, holder));
/*
* Note that for a whole device bd_holders
This commit added accessing bdev->bd_mutex before checking res, which
will cause use-after-free. So I think the fixes tag should be:
Fixes: 77ea887e433a ("implement in-kernel gendisk events handling")
> Otherwise the patch looks good to me.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> regards,
> dan carpenter
>
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6] block: Fix use-after-free in blkdev_get()
2020-06-16 11:24 ` Jason Yan
@ 2020-06-16 11:49 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-06-16 11:49 UTC (permalink / raw)
To: Jason Yan
Cc: axboe, linux-block, linux-fsdevel, Christoph Hellwig, Ming Lei,
Jan Kara, Hulk Robot, Sedat Dilek
On Tue, Jun 16, 2020 at 07:24:07PM +0800, Jason Yan wrote:
> This commit added accessing bdev->bd_mutex before checking res, which will
> cause use-after-free. So I think the fixes tag should be:
>
> Fixes: 77ea887e433a ("implement in-kernel gendisk events handling")
Yeah. That looks right. I'm surprised it goes back so far.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-16 11:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16 3:40 [PATCH v6] block: Fix use-after-free in blkdev_get() Jason Yan
2020-06-16 10:20 ` Dan Carpenter
2020-06-16 11:24 ` Jason Yan
2020-06-16 11:49 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.