On Sat, 20 Aug 2022 17:11:05 +0530 Siddh Raman Pant wrote: > Syzkaller has reported a warning in iomap_iter(), which got > triggered due to a call to iomap_iter_done() which has: > WARN_ON_ONCE(iter->iomap.offset > iter->pos); > > The warning was triggered because `pos` was being negative. > I was having offset = 0, pos = -2950420705881096192. > > This ridiculously negative value smells of an overflow, and sure > it is. > > The userspace can configure a loop using an ioctl call, wherein > a configuration of type loop_config is passed (see lo_ioctl()'s > case on line 1550 of drivers/block/loop.c). This proceeds to call > loop_configure() which in turn calls loop_set_status_from_info() > (see line 1050 of loop.c), passing &config->info which is of type > loop_info64*. This function then sets the appropriate values, like > the offset. > > The problem here is loop_device has lo_offset of type loff_t > (see line 52 of loop.c), which is typdef-chained to long long, > whereas loop_info64 has lo_offset of type __u64 (see line 56 of > include/uapi/linux/loop.h). > > The function directly copies offset from info to the device as > follows (See line 980 of loop.c): > lo->lo_offset = info->lo_offset; > > This results in the encountered overflow (in my case, the RHS > was 15496323367828455424). > > Thus, convert the type definitions in loop_info64 to their > signed counterparts in order to match definitions in loop_device, > and check for negative value during loop_set_status_from_info(). > > Bug report: https://syzkaller.appspot.com/bug?id=c620fe14aac810396d3c3edc9ad73848bf69a29e > Reported-and-tested-by: syzbot+a8e049cd3abd342936b6@xxxxxxxxxxxxxxxxxxxxxxxxx > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: Siddh Raman Pant code@xxxxxxxx> > --- > Unless I am missing any other uses or quirks of UAPI loop_info64, > I think this won't introduce regression, since if something is > working, it will continue to work. If something does break, then > it was relying on overflows, which is anyways an incorrect way > to go about. > > Also, it seems even the 32-bit compatibility structure uses the > signed types (compat_loop_info uses compat_int_t which is s32), > so this patch should be fine. > > 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 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 There has been discussion on syzkaller mailing list: https://groups.google.com/g/syzkaller-bugs/c/bg3ANn_7oJw/m/-MbtBx9cAwAJ Reproducing the latest reply: 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. > > > Also, it seems even the 32-bit compatibility structure uses signed > > types. > > We should probably fix that as well. Thus, I will send a v2 once the discussion is resolved. I had sent this patch because the discussion was stale for 2 days and Matthew seemed to be active on other email threads. Thanks, Siddh
