Re: [syzbot] WARNING in iomap_iter

Re: [syzbot] WARNING in iomap_iter

[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

On Mon, Nov 08, 2021 at 03:46:27PM -0800, syzbot wrote:
> 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+a8e049cd3abd342936b6@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 syzkaller@googlegroups.com.
> 
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
---end quoted text---

Re: [syzbot] WARNING in iomap_iter

[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+a8e049cd3abd342936b6@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>

Re: [syzbot] WARNING in iomap_iter

[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

>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+a8e049cd3abd342936b6@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>
>

Re: [syzbot] WARNING in iomap_iter

[Dave Chinner]

On Sun, Feb 13, 2022 at 08:04:10PM +0530, Ritesh Harjani wrote:
> 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?

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
david@fromorbit.com

Re: [syzbot] WARNING in iomap_iter

[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

Re: [syzbot] WARNING in iomap_iter

[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
--- 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

Re: [syzbot] WARNING in iomap_iter

[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;

Re: [syzbot] WARNING in iomap_iter

[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

Re: [syzbot] WARNING in iomap_iter

[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.

Re: [syzbot] WARNING in iomap_iter

[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

Re: [syzbot] WARNING in iomap_iter

[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

Re: [syzbot] WARNING in iomap_iter

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+a8e049cd3abd342936b6@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

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

Re: [syzbot] WARNING in iomap_iter

[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.

Re: [syzbot] WARNING in iomap_iter

[Siddh Raman Pant]

On Mon, 22 Aug 2022 20:15:28 +0530  Matthew Wilcox  wrote:
> 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.

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

Re: [syzbot] WARNING in iomap_iter

[Matthew Wilcox]

On Mon, Aug 22, 2022 at 08:19:43PM +0530, Siddh Raman Pant wrote:
> On Mon, 22 Aug 2022 20:15:28 +0530  Matthew Wilcox  wrote:
> > 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.
> 
> 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.

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.

Re: [syzbot] WARNING in iomap_iter

[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

Re: [syzbot] WARNING in iomap_iter

[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

Re: [syzbot] WARNING in iomap_iter

[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?

> +		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
> 
> 

Re: [syzbot] WARNING in iomap_iter

[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)
+		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

Re: [syzbot] WARNING in iomap_iter

[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) <willy@infradead.org>

> ---
>  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)
> +		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
> 
> 

Re: [syzbot] WARNING in iomap_iter

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+a8e049cd3abd342936b6@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

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

Re: [syzbot] WARNING in iomap_iter

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+a8e049cd3abd342936b6@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

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

Re: [syzbot] WARNING in iomap_iter

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+a8e049cd3abd342936b6@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.

Re: [syzbot] WARNING in iomap_iter

[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