Re: [PATCH] loop: Correct UAPI definitions to match with driver

From: Siddh Raman Pant
Date: Sun Aug 21 2022 - 07:24:07 EST


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