[syzbot] WARNING in iomap_iter

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 7ddb58cb0eca Merge tag 'clk-for-linus' of git://git.kernel..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13443b82b00000
kernel config: https://syzkaller.appspot.com/x/.config?x=a30ce238f371e547
dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: i386

Unfortunately, I don't have any reproducer for this issue yet.

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+a8e049...@syzkaller.appspotmail.com

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1112 at fs/iomap/iter.c:33 iomap_iter_done fs/iomap/iter.c:33 [inline]
WARNING: CPU: 0 PID: 1112 at fs/iomap/iter.c:33 iomap_iter+0xdcf/0x11b0 fs/iomap/iter.c:78
Modules linked in:
CPU: 0 PID: 1112 Comm: kworker/u4:5 Not tainted 5.15.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: loop0 loop_rootcg_workfn
RIP: 0010:iomap_iter_done fs/iomap/iter.c:33 [inline]
RIP: 0010:iomap_iter+0xdcf/0x11b0 fs/iomap/iter.c:78
Code: fd ff ff e8 93 9f d1 ff e9 f9 f9 ff ff e8 79 24 8b ff 0f 0b e9 85 f8 ff ff e8 6d 24 8b ff 0f 0b e9 96 f7 ff ff e8 61 24 8b ff <0f> 0b e9 f8 f6 ff ff e8 55 24 8b ff 0f 0b 48 b8 00 00 00 00 00 fc
RSP: 0018:ffffc90004d4f680 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffffc90004d4f818 RCX: 0000000000000000
RDX: ffff88801c181d00 RSI: ffffffff81ec9faf RDI: 0000000000000003
RBP: ffffc90004d4f848 R08: 00000fff80000000 R09: 000000000000000c
R10: ffffffff81ec96a0 R11: 000000000000003f R12: ffffc90004d4f820
R13: ffffffff80000000 R14: ffffc90004d4f840 R15: ffffc90004d4f888
FS: 0000000000000000(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fc3d2148008 CR3: 00000000307d5000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
__iomap_dio_rw+0x6b2/0x1a10 fs/iomap/direct-io.c:588
iomap_dio_rw+0x38/0x90 fs/iomap/direct-io.c:679
ext4_dio_read_iter fs/ext4/file.c:77 [inline]
ext4_file_read_iter+0x41c/0x5d0 fs/ext4/file.c:128
call_read_iter include/linux/fs.h:2155 [inline]
lo_rw_aio.isra.0+0xa99/0xc90 drivers/block/loop.c:453
do_req_filebacked drivers/block/loop.c:497 [inline]
loop_handle_cmd drivers/block/loop.c:1857 [inline]
loop_process_work+0x92f/0x1db0 drivers/block/loop.c:1897
process_one_work+0x9b2/0x1690 kernel/workqueue.c:2298
worker_thread+0x658/0x11f0 kernel/workqueue.c:2445
kthread+0x405/0x4f0 kernel/kthread.c:327
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[Christoph Hellwig]

This is:

WARN_ON_ONCE(iter->iomap.offset > iter->pos);

so it looks like ext4 has an issue in its ->iomap_begin implementation

---end quoted text---

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit: 83e396641110 Merge tag 'soc-fixes-5.17-1' of git://git.ker..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11fe01a4700000
kernel config: https://syzkaller.appspot.com/x/.config?x=88e0a6a3dbf057cf
dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14f8cad2700000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=132c16ba700000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+a8e049...@syzkaller.appspotmail.com

------------[ cut here ]------------

WARNING: CPU: 1 PID: 10 at fs/iomap/iter.c:33 iomap_iter_done fs/iomap/iter.c:33 [inline]
WARNING: CPU: 1 PID: 10 at fs/iomap/iter.c:33 iomap_iter+0x7ca/0x890 fs/iomap/iter.c:78
Modules linked in:
CPU: 1 PID: 10 Comm: kworker/u4:1 Not tainted 5.17.0-rc3-syzkaller-00247-g83e396641110 #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: loop0 loop_rootcg_workfn
RIP: 0010:iomap_iter_done fs/iomap/iter.c:33 [inline]

RIP: 0010:iomap_iter+0x7ca/0x890 fs/iomap/iter.c:78
Code: e8 3b 81 83 ff eb 0c e8 34 81 83 ff eb 05 e8 2d 81 83 ff 44 89 e8 48 83 c4 40 5b 41 5c 41 5d 41 5e 41 5f 5d c3 e8 16 81 83 ff <0f> 0b e9 9e fe ff ff e8 0a 81 83 ff 0f 0b e9 d0 fe ff ff e8 fe 80
RSP: 0018:ffffc90000cf73c8 EFLAGS: 00010293
RAX: ffffffff82022d4a RBX: ffffffff80000000 RCX: ffff888011fe9d00
RDX: 0000000000000000 RSI: ffffffff80000000 RDI: 00000fff80000000
RBP: 00000fff80000000 R08: ffffffff82022be1 R09: ffffed100fd4dc19
R10: ffffed100fd4dc19 R11: 0000000000000000 R12: ffffc90000cf75c8
R13: 1ffff9200019eebe R14: 1ffff9200019eeb9 R15: ffffc90000cf75f0
FS: 0000000000000000(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00007fbf80df2b88 CR3: 000000007e8f6000 CR4: 00000000003506e0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>

__iomap_dio_rw+0xa8e/0x1e00 fs/iomap/direct-io.c:589
iomap_dio_rw+0x38/0x80 fs/iomap/direct-io.c:680
ext4_dio_read_iter fs/ext4/file.c:77 [inline]
ext4_file_read_iter+0x52f/0x6c0 fs/ext4/file.c:128
lo_rw_aio+0xc75/0x1060
loop_handle_cmd drivers/block/loop.c:1846 [inline]
loop_process_work+0x6a4/0x22b0 drivers/block/loop.c:1886
process_one_work+0x850/0x1130 kernel/workqueue.c:2307
worker_thread+0xab1/0x1300 kernel/workqueue.c:2454
kthread+0x2a3/0x2d0 kernel/kthread.c:377
ret_from_fork+0x1f/0x30
</TASK>

[Ritesh Harjani]

On 22/02/12 12:41PM, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
>
> HEAD commit: 83e396641110 Merge tag 'soc-fixes-5.17-1' of git://git.ker..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=11fe01a4700000
> kernel config: https://syzkaller.appspot.com/x/.config?x=88e0a6a3dbf057cf
> dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
> compiler: Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14f8cad2700000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=132c16ba700000

FYI - I could reproduce with above C reproduer on my setup 5.17-rc3.
I was also able to hit it with XFS <below stack shows that>

So here is some initial analysis on this one. I haven't completely debugged it
though. I am just putting my observations here for others too.

It seems iomap_dio_rw is getting called with a negative iocb->ki_pos value.
(I haven't yet looked into when can this happen. Is it due to negative loop
device mapping range offset or something?)

i.e.
(gdb) p iocb->ki_pos
$101 = -2147483648
(gdb) p /x iocb->ki_pos
$102 = 0xffffffff80000000
(gdb)

This when passed to ->iomap_begin() sometimes is resulting into iomap->offset
which is a positive value and hence hitting below warn_on_once in
iomap_iter_done().

WARN_ON_ONCE(iter->iomap.offset > iter->pos)

1. So I think the question here is what does it mean when xfs/ext4_file_read_iter()
is called with negative iocb->ki_pos value?
2. Also when can iocb->ki_pos be negative?

<Stack Track on XFS>
======================

[ 998.417802] ------------[ cut here ]------------
[ 998.420195] WARNING: CPU: 0 PID: 1579 at fs/iomap/iter.c:33
iomap_iter+0x301/0x320
[ 998.424610] Modules linked in:
[ 998.425683] CPU: 0 PID: 1579 Comm: kworker/u2:5 Tainted:
G W 5.17.0-rc3+ #0
[ 998.428085] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.13.0-1ubuntu1 04
[ 998.430830] Workqueue: loop0 loop_rootcg_workfn
[ 998.432300] RIP: 0010:iomap_iter+0x301/0x320
[ 998.433647] Code: 89 f2 e8 72 f1 ff ff 65 ff 0d bb d0 ce 7e 0f 85 c4 fe ff ff
e8 2f 3e cdc
[ 998.438518] RSP: 0018:ffffc90000c13b30 EFLAGS: 00010307
[ 998.440490] RAX: 0000000000010000 RBX: ffffc90000c13bc0 RCX: 000000000000000c
[ 998.442576] RDX: ffffffff80000000 RSI: 0000000000001000 RDI: 0000000000000000
[ 998.444625] RBP: ffffc90000c13b50 R08: 0000000000000003 R09: ffff88814ceb9b00
[ 998.446768] R10: ffff88815122e000 R11: 000000000000000f R12: ffffffff82657c90
[ 998.453038] R13: ffffc90000c13be8 R14: ffffc90000c13c30 R15: ffffffff82657c90
[ 998.455533] FS: 0000000000000000(0000) GS:ffff88852bc00000(0000)
knlGS:0000000000000000
[ 998.458136] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 998.460069] CR2: 00007ffff4443000 CR3: 0000000105e7e000 CR4: 00000000000006f0
[ 998.462447] Call Trace:
[ 998.463108] <TASK>
[ 998.464510] __iomap_dio_rw+0x25b/0x840
[ 998.466005] iomap_dio_rw+0xe/0x30
[ 998.467476] xfs_file_dio_read+0xb9/0xf0
[ 998.469044] xfs_file_read_iter+0xc1/0xe0
[ 998.470623] lo_rw_aio+0x27a/0x2a0
[ 998.472042] loop_process_work+0x2c7/0x8c0
[ 998.473621] ? finish_task_switch+0xbc/0x260
[ 998.475232] ? __switch_to+0x2cf/0x480
[ 998.476832] loop_rootcg_workfn+0x1b/0x20
[ 998.478431] process_one_work+0x1b7/0x380
[ 998.479958] worker_thread+0x4d/0x380
[ 998.481440] ? process_one_work+0x380/0x380
[ 998.482992] kthread+0xff/0x130
[ 998.484420] ? kthread_complete_and_exit+0x20/0x20
[ 998.486122] ret_from_fork+0x22/0x30
[ 998.487616] </TASK>
[ 998.488199] ---[ end trace 0000000000000000 ]---


-ritesh

[Dave Chinner]

Sounds like a bug in the loop driver, not a problem with the iomap
DIO code. The IO path normally checks the position via
rw_verify_area() high up in the IO path, so by the time iocb->ki_pos
gets to filesystems and low level IO routines it's supposed to have
already been checked against overflows. Looks to me like the loop
driver is not checking the back end file position it calculates for
overflows...

Cheers,

Dave.
--
Dave Chinner
da...@fromorbit.com

[Siddh Raman Pant]

This is probably due to mismatch in types between userspace API struct
and the kernel's internal struct, which leads to offset being overflowed
after getting converted from __u64 (unsigned long long) to loff_t (signed
long long), resulting in ridiculously negative offset value.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---
include/uapi/linux/loop.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index 6f63527dd2ed..33c07c467da4 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -53,12 +53,12 @@ struct loop_info64 {
__u64 lo_device; /* ioctl r/o */
__u64 lo_inode; /* ioctl r/o */
__u64 lo_rdevice; /* ioctl r/o */
- __u64 lo_offset;
- __u64 lo_sizelimit;/* bytes, 0 == max available */
- __u32 lo_number; /* ioctl r/o */
- __u32 lo_encrypt_type; /* obsolete, ignored */
- __u32 lo_encrypt_key_size; /* ioctl w/o */
- __u32 lo_flags;
+ __s64 lo_offset;
+ __s64 lo_sizelimit; /* bytes, 0 == max available */
+ __s32 lo_number; /* ioctl r/o */
+ __s32 lo_encrypt_type; /* obsolete, ignored */
+ __s32 lo_encrypt_key_size; /* ioctl w/o */
+ __s32 lo_flags;
__u8 lo_file_name[LO_NAME_SIZE];
__u8 lo_crypt_name[LO_NAME_SIZE];
__u8 lo_encrypt_key[LO_KEY_SIZE]; /* ioctl w/o */
--
2.35.1

[Siddh Raman Pant]

The last test patch accidentally left out less-than-zero checks...

Is there a way to cancel previously requested tests?

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---

drivers/block/loop.c | 3 +++
include/uapi/linux/loop.h | 12 ++++++------
2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e3c0ba93c1a3..4ca20ce3158d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -977,6 +977,9 @@ loop_set_status_from_info(struct loop_device *lo,
return -EINVAL;
}

+ if (info->lo_offset < 0 || info->lo_sizelimit < 0)
+ return -EINVAL;
+
lo->lo_offset = info->lo_offset;
lo->lo_sizelimit = info->lo_sizelimit;
memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index 6f63527dd2ed..973565f38f9d 100644

[Matthew Wilcox]

On Thu, Aug 18, 2022 at 04:41:17PM +0530, Siddh Raman Pant wrote:
> include/uapi/linux/loop.h | 12 ++++++------

I don't think changing these from u64 to s64 is the right way to go.

> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index e3c0ba93c1a3..4ca20ce3158d 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -977,6 +977,9 @@ loop_set_status_from_info(struct loop_device *lo,
> return -EINVAL;
> }
>
> + if (info->lo_offset < 0 || info->lo_sizelimit < 0)
> + return -EINVAL;
> +
> lo->lo_offset = info->lo_offset;
> lo->lo_sizelimit = info->lo_sizelimit;

I'd instead do it here:

if (lo>lo_offset < 0 || lo->lo_sizelimit < 0)
return -EINVAL;

[Siddh Raman Pant]

On Thu, 18 Aug 2022 20:20:02 +0530 Matthew Wilcox wrote:
> I don't think changing these from u64 to s64 is the right way to go.

Why do you think so? Is there somnething I overlooked?

I think it won't intorduce regression, since if something is working,
it will continue to work. If something does break, then they were
relying on overflows, which is anyways an incorrect way to go about.

Also, it seems even the 32-bit compatibility structure uses signed
types.

Thanks,
Siddh

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in iomap_iter

------------[ cut here ]------------
WARNING: CPU: 1 PID: 11 at fs/iomap/iter.c:33 iomap_iter_done fs/iomap/iter.c:33 [inline]
WARNING: CPU: 1 PID: 11 at fs/iomap/iter.c:33 iomap_iter+0xd8c/0x1100 fs/iomap/iter.c:78
Modules linked in:
CPU: 1 PID: 11 Comm: kworker/u4:1 Not tainted 6.0.0-rc1-syzkaller-00067-g573ae4f13f63-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
Workqueue: loop3 loop_workfn

RIP: 0010:iomap_iter_done fs/iomap/iter.c:33 [inline]

RIP: 0010:iomap_iter+0xd8c/0x1100 fs/iomap/iter.c:78
Code: ff e8 28 60 87 ff 0f 0b e9 f1 f9 ff ff e8 1c 60 87 ff 0f 0b e9 86 f7 ff ff e8 10 60 87 ff 0f 0b e9 5e f7 ff ff e8 04 60 87 ff <0f> 0b e9 1a f7 ff ff e8 f8 5f 87 ff e8 73 b4 8a 07 31 ff 89 c5 89
RSP: 0018:ffffc90000107668 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffffc90000107800 RCX: 0000000000000000
RDX: ffff888011a9bb00 RSI: ffffffff81f4ab4c RDI: 0000000000000006
RBP: 0000000000000000 R08: 0000000000000006 R09: 0000000000000000
R10: d70e000000000000 R11: 0000000000000004 R12: 0000000000000000
R13: d70e000000000000 R14: ffffc90000107828 R15: ffffc90000107870

FS: 0000000000000000(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00007fae015980a0 CR3: 000000007f2cd000 CR4: 0000000000350ee0
Call Trace:
<TASK>
__iomap_dio_rw+0x6c6/0x1c20 fs/iomap/direct-io.c:601
iomap_dio_rw+0x3c/0xa0 fs/iomap/direct-io.c:690
ext4_dio_read_iter fs/ext4/file.c:79 [inline]
ext4_file_read_iter+0x434/0x600 fs/ext4/file.c:130
call_read_iter include/linux/fs.h:2181 [inline]
lo_rw_aio.isra.0+0xa54/0xc50 drivers/block/loop.c:454
do_req_filebacked drivers/block/loop.c:498 [inline]
loop_handle_cmd drivers/block/loop.c:1859 [inline]
loop_process_work+0x969/0x2050 drivers/block/loop.c:1894
process_one_work+0x991/0x1610 kernel/workqueue.c:2289
worker_thread+0x665/0x1080 kernel/workqueue.c:2436
kthread+0x2e4/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
</TASK>


Tested on:

commit: 573ae4f1 tee: add overflow check in register_shm_helpe..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=11c4af0d080000
kernel config: https://syzkaller.appspot.com/x/.config?x=d9d854f607a68b32
dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=13ba8e5b080000

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+a8e049...@syzkaller.appspotmail.com

Tested on:

commit: 573ae4f1 tee: add overflow check in register_shm_helpe..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=1334aaeb080000

kernel config: https://syzkaller.appspot.com/x/.config?x=d9d854f607a68b32
dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=117e96d3080000

Note: testing is done by a robot and is best-effort only.

[hch]

On Thu, Aug 18, 2022 at 08:51:16PM +0530, Siddh Raman Pant wrote:
> On Thu, 18 Aug 2022 20:20:02 +0530 Matthew Wilcox wrote:
> > I don't think changing these from u64 to s64 is the right way to go.
>
> Why do you think so? Is there somnething I overlooked?
>
> I think it won't intorduce regression, since if something is working,
> it will continue to work. If something does break, then they were
> relying on overflows, which is anyways an incorrect way to go about.

Well, for example userspace code expecting unsignedness of these
types could break. So if we really think changing the types is so
much preferred we'd need to audit common userspace first. Because
of that I think the version proposed by willy is generally preferred.

> Also, it seems even the 32-bit compatibility structure uses signed
> types.

We should probably fix that as well.

[Siddh Raman Pant]

On Sun, 21 Aug 2022 11:59:05 +0530 Christoph Hellwig wrote:
> On Thu, Aug 18, 2022 at 08:51:16PM +0530, Siddh Raman Pant wrote:
> > On Thu, 18 Aug 2022 20:20:02 +0530 Matthew Wilcox wrote:
> > > I don't think changing these from u64 to s64 is the right way to go.
> >
> > Why do you think so? Is there somnething I overlooked?
> >
> > I think it won't intorduce regression, since if something is working,
> > it will continue to work. If something does break, then they were
> > relying on overflows, which is anyways an incorrect way to go about.
>
> Well, for example userspace code expecting unsignedness of these
> types could break. So if we really think changing the types is so
> much preferred we'd need to audit common userspace first. Because
> of that I think the version proposed by willy is generally preferred.

Alright.

> > Also, it seems even the 32-bit compatibility structure uses signed
> > types.
>
> We should probably fix that as well.

Isn't having signed type how it is should be though? Or do you mean need
to fix assignment in the conversions (like in loop_info64_from_compat)?

Thanks,
Siddh

[Siddh Raman Pant]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---

drivers/block/loop.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e3c0ba93c1a3..a3d9af0a2077 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -979,9 +979,15 @@ loop_set_status_from_info(struct loop_device *lo,

lo->lo_offset = info->lo_offset;
lo->lo_sizelimit = info->lo_sizelimit;

+ lo->lo_flags = info->lo_flags;
+
+ /* loff_t/int vars are assigned __u64/__u32 vars (respectively) */
+ if (lo->lo_offset < 0 || lo->lo_sizelimit < 0 || lo->lo_flags < 0)
+ return -EOVERFLOW;
+

memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);

lo->lo_file_name[LO_NAME_SIZE-1] = 0;
- lo->lo_flags = info->lo_flags;
+
return 0;
}

--
2.35.1

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+a8e049...@syzkaller.appspotmail.com

Tested on:

commit: e3f259d3 Merge tag 'i2c-for-6.0-rc2' of git://git.kern..

git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=16085295080000
kernel config: https://syzkaller.appspot.com/x/.config?x=3babfbf8c1ad1951

dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=13c53aa5080000

[Matthew Wilcox]

On Sun, Aug 21, 2022 at 05:18:16PM +0530, Siddh Raman Pant wrote:
> @@ -979,9 +979,15 @@ loop_set_status_from_info(struct loop_device *lo,
>
> lo->lo_offset = info->lo_offset;
> lo->lo_sizelimit = info->lo_sizelimit;
> + lo->lo_flags = info->lo_flags;
> +
> + /* loff_t/int vars are assigned __u64/__u32 vars (respectively) */
> + if (lo->lo_offset < 0 || lo->lo_sizelimit < 0 || lo->lo_flags < 0)
> + return -EOVERFLOW;

Why would you check lo_flags? That really, really should be an unsigned
type.

[Siddh Raman Pant]

I agree, but the loop_device struct has (see line 54 of loop.c):
int lo_flags;

Thus, I checked for it, as we are not changing any types.

Thanks,
Siddh

[Matthew Wilcox]

But it's not an integer. It's a bitfield. Nobody checks lo_flags for
"is it less than zero". That makes it very different from lo_offset.

[Siddh Raman Pant]

On Mon, 22 Aug 2022 20:22:32 +0530 Matthew Wilcox wrote:
> But it's not an integer. It's a bitfield. Nobody checks lo_flags for
> "is it less than zero". That makes it very different from lo_offset.

Thanks for clarifying, I see where I was wrong. I overlooked its use as a
bitfield.

Thanks,
Siddh

[Siddh Raman Pant]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---

drivers/block/loop.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e3c0ba93c1a3..e1fe8eda020f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -979,6 +979,11 @@ loop_set_status_from_info(struct loop_device *lo,

lo->lo_offset = info->lo_offset;
lo->lo_sizelimit = info->lo_sizelimit;
+

+ /* loff_t vars have been assigned __u64 */

+ if (lo->lo_offset < 0 || lo->lo_sizelimit < 0 || lo->lo_flags < 0)
+ return -EOVERFLOW;

+
memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
lo->lo_file_name[LO_NAME_SIZE-1] = 0;

lo->lo_flags = info->lo_flags;

--
2.35.1

[Matthew Wilcox]

On Tue, Aug 23, 2022 at 08:51:01PM +0530, Siddh Raman Pant wrote:
> + if (lo->lo_offset < 0 || lo->lo_sizelimit < 0 || lo->lo_flags < 0)

The lo_flags check is still there?

[Siddh Raman Pant]

Oof, I didn't mean it to be there. That would actually be wrong anyways.

Extremely sorry for the avoidable oversight,
Siddh

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---
drivers/block/loop.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e3c0ba93c1a3..e1fe8eda020f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -979,6 +979,11 @@ loop_set_status_from_info(struct loop_device *lo,

lo->lo_offset = info->lo_offset;
lo->lo_sizelimit = info->lo_sizelimit;
+
+ /* loff_t vars have been assigned __u64 */

+ if (lo->lo_offset < 0 || lo->lo_sizelimit < 0)

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+a8e049...@syzkaller.appspotmail.com

Tested on:

commit: 072e5135 Merge tag 'nfs-for-5.20-2' of git://git.linux..

git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=120a311d080000
kernel config: https://syzkaller.appspot.com/x/.config?x=3f885f57a0f25c38

dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=169d8e5b080000

[Matthew Wilcox]

On Tue, Aug 23, 2022 at 09:05:42PM +0530, Siddh Raman Pant wrote:
> Oof, I didn't mean it to be there. That would actually be wrong anyways.
>
> Extremely sorry for the avoidable oversight,
> Siddh
>
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Reviewed-by: Matthew Wilcox (Oracle) <wi...@infradead.org>

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+a8e049...@syzkaller.appspotmail.com

Tested on:

commit: 072e5135 Merge tag 'nfs-for-5.20-2' of git://git.linux..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=123599b5080000

kernel config: https://syzkaller.appspot.com/x/.config?x=3f885f57a0f25c38
dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=160ef0a3080000

[Siddh Raman Pant]

Syzkaller posted a new reproducer unrelated to the issue causing
the older crash under this same issue, since the same function
triggers the newer warning.

This time it is related to erofs setting length equal to zero in
z_erofs_iomap_begin_report().

Thanks,
Siddh