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