Message-ID: <182c024c8f1.2e77843173026.4408233265123165366@siddh.me>
Date:   Sun, 21 Aug 2022 16:53:32 +0530
From:   Siddh Raman Pant <code@...dh.me>
To:     "Jens Axboe" <axboe@...nel.dk>,
        "Carlos Llamas" <cmllamas@...gle.com>
Cc:     "linux-block" <linux-block@...r.kernel.org>,
        "linux-kernel" <linux-kernel@...r.kernel.org>,
        "linux-kernel-mentees" 
        <linux-kernel-mentees@...ts.linuxfoundation.org>,
        "syzbot+a8e049cd3abd342936b6" 
        <syzbot+a8e049cd3abd342936b6@...kaller.appspotmail.com>,
        "stable" <stable@...r.kernel.org>
Subject: Re: [PATCH] loop: Correct UAPI definitions to match with driver

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@...kaller.appspotmail.com
> Cc: stable@...r.kernel.org
> Signed-off-by: Siddh Raman Pant code@...dh.me>
> ---
> 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

Powered by blists - more mailing lists