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
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;
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
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
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.
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.