Re: [PATCH 5.14 018/334] nbd: add the check to prevent overflow in __nbd_ioctl()

[Naresh Kamboju]

On Mon, 13 Sept 2021 at 19:51, Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
>
> From: Baokun Li <liba...@huawei.com>
>
> [ Upstream commit fad7cd3310db3099f95dd34312c77740fbc455e5 ]
>
> If user specify a large enough value of NBD blocks option, it may trigger
> signed integer overflow which may lead to nbd->config->bytesize becomes a
> large or small value, zero in particular.
>
> UBSAN: Undefined behaviour in drivers/block/nbd.c:325:31
> signed integer overflow:
> 1024 * 4611686155866341414 cannot be represented in type 'long long int'
> [...]
> Call trace:
> [...]
> handle_overflow+0x188/0x1dc lib/ubsan.c:192
> __ubsan_handle_mul_overflow+0x34/0x44 lib/ubsan.c:213
> nbd_size_set drivers/block/nbd.c:325 [inline]
> __nbd_ioctl drivers/block/nbd.c:1342 [inline]
> nbd_ioctl+0x998/0xa10 drivers/block/nbd.c:1395
> __blkdev_driver_ioctl block/ioctl.c:311 [inline]
> [...]
>
> Although it is not a big deal, still silence the UBSAN by limit
> the input value.
>
> Reported-by: Hulk Robot <hul...@huawei.com>
> Signed-off-by: Baokun Li <liba...@huawei.com>
> Reviewed-by: Josef Bacik <jo...@toxicpanda.com>
> Link: https://lore.kernel.org/r/20210804021212.9...@huawei.com
> [axboe: dropped unlikely()]
> Signed-off-by: Jens Axboe <ax...@kernel.dk>
> Signed-off-by: Sasha Levin <sas...@kernel.org>
> ---
> drivers/block/nbd.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 19f5d5a8b16a..acf3f85bf3c7 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1388,6 +1388,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> unsigned int cmd, unsigned long arg)
> {
> struct nbd_config *config = nbd->config;
> + loff_t bytesize;
>
> switch (cmd) {
> case NBD_DISCONNECT:
> @@ -1402,8 +1403,9 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> case NBD_SET_SIZE:
> return nbd_set_size(nbd, arg, config->blksize);
> case NBD_SET_SIZE_BLOCKS:
> - return nbd_set_size(nbd, arg * config->blksize,
> - config->blksize);
> + if (check_mul_overflow((loff_t)arg, config->blksize, &bytesize))
> + return -EINVAL;
> + return nbd_set_size(nbd, bytesize, config->blksize);
> case NBD_SET_TIMEOUT:
> nbd_set_cmd_timeout(nbd, arg);
> return 0;

arm clang-10, clang-11, clang-12 and clang-13 builds failed.
due to this commit on 5.14 and 5.13 on following configs,
- footbridge_defconfig
- mini2440_defconfig
- s3c2410_defconfig

This was already reported on the mailing list.

ERROR: modpost: "__mulodi4" [drivers/block/nbd.ko] undefined! #1438
https://github.com/ClangBuiltLinux/linux/issues/1438

[PATCH 00/10] raise minimum GCC version to 5.1
https://lore.kernel.org/lkml/20210910234047.101...@google.com/

linux-next: build failure while building Linus' tree
https://lore.kernel.org/all/20210909182...@canb.auug.org.au/

Full build log,
https://gitlab.com/Linaro/lkft/mirrors/stable/linux-stable-rc/-/jobs/1585407346#L1111


--
Linaro LKFT
https://lkft.linaro.org

[Greg Kroah-Hartman]

Has anyone submitted a fix for this upstream yet? I can't seem to find
one :(

[Nick Desaulniers]

> Has anyone submitted a fix for this upstream yet? I can't seem to find
> one :(

That lore link has a series to address this, though that's maybe
something we don't want to backport to stable.

I thought about this all weekend; I think I might be able to work
around the one concern I had with my other approach, using
__builtin_choose_expr().

There's an issue with my alternative approach
(https://gist.github.com/nickdesaulniers/2479818f4983bbf2d688cebbab435863)
with declaring the local variable z in div_64() since either operand
could be 64b, which result in an unwanted truncation if the dividend
is 32b (or less, and divisor is 64b). I think (what I realized this
weekend) is that we might be able to replace the `if` with
`__builtin_choose_expr`, then have that whole expression be the final
statement and thus the "return value" of the statement expression.

I need to play with that idea more; maybe that could be a more
manageable patch for stable. But I also need to eat lunch, and I've
been in the Rust for Linux conference, and trying to organize 3 other
conferences for the next two weeks...
--
Thanks,
~Nick Desaulniers

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 11:39 AM Nick Desaulniers
<ndesau...@google.com> wrote:
>
> On Mon, Sep 13, 2021 at 10:58 AM Greg Kroah-Hartman
> <gre...@linuxfoundation.org> wrote:
> >
> > On Mon, Sep 13, 2021 at 09:52:33PM +0530, Naresh Kamboju wrote:
> > > [PATCH 00/10] raise minimum GCC version to 5.1
> > > https://lore.kernel.org/lkml/20210910234047.101...@google.com/
> >
> > Has anyone submitted a fix for this upstream yet? I can't seem to find
> > one :(
>
> That lore link has a series to address this, though that's maybe
> something we don't want to backport to stable.
>
> I thought about this all weekend; I think I might be able to work
> around the one concern I had with my other approach, using
> __builtin_choose_expr().
>
> There's an issue with my alternative approach
> (https://gist.github.com/nickdesaulniers/2479818f4983bbf2d688cebbab435863)
> with declaring the local variable z in div_64() since either operand
> could be 64b, which result in an unwanted truncation if the dividend
> is 32b (or less, and divisor is 64b). I think (what I realized this
> weekend) is that we might be able to replace the `if` with
> `__builtin_choose_expr`, then have that whole expression be the final
> statement and thus the "return value" of the statement expression.

Christ...that...works? Though, did Linus just merge my patches for gcc 5.1?

Anyways, I'll send something like this for stable:
---

diff --git a/include/linux/math64.h b/include/linux/math64.h
index 2928f03d6d46..e9ab8c25f8d3 100644
--- a/include/linux/math64.h
+++ b/include/linux/math64.h
@@ -11,6 +11,9 @@

#define div64_long(x, y) div64_s64((x), (y))
#define div64_ul(x, y) div64_u64((x), (y))
+#ifndef is_signed_type
+#define is_signed_type(type) (((type)(-1)) < (type)1)
+#endif

/**
* div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
@@ -112,6 +115,15 @@ extern s64 div64_s64(s64 dividend, s64 divisor);

#endif /* BITS_PER_LONG */

+#define div64_x64(dividend, divisor) ({ \
+ BUILD_BUG_ON_MSG(sizeof(dividend) < sizeof(u64),\
+ "prefer div_x64"); \
+ __builtin_choose_expr( \
+ is_signed_type(typeof(dividend)), \
+ div64_s64(dividend, divisor), \
+ div64_u64(dividend, divisor)); \
+})
+
/**
* div_u64 - unsigned 64bit divide with 32bit divisor
* @dividend: unsigned 64bit dividend
@@ -142,6 +154,28 @@ static inline s64 div_s64(s64 dividend, s32 divisor)
}
#endif

+#define div_x64(dividend, divisor) ({ \
+ BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u32),\
+ "prefer div64_x64"); \
+ __builtin_choose_expr( \
+ is_signed_type(typeof(dividend)), \
+ div_s64(dividend, divisor), \
+ div_u64(dividend, divisor)); \
+})
+
+// TODO: what if divisor is 128b?
+#define div_64(dividend, divisor) ({
\
+ __builtin_choose_expr(
\
+ __builtin_types_compatible_p(typeof(dividend), s64) ||
\
+ __builtin_types_compatible_p(typeof(dividend), u64),
\
+ __builtin_choose_expr(
\
+ __builtin_types_compatible_p(typeof(divisor),
s64) || \
+ __builtin_types_compatible_p(typeof(divisor),
u64), \
+ div64_x64(dividend, divisor),
\
+ div_x64(dividend, divisor)),
\
+ dividend / divisor);
\
+})
+
u32 iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder);

#ifndef mul_u32_u32
---
--
Thanks,
~Nick Desaulniers

[Sedat Dilek]

"Merge branch 'gcc-min-version-5.1' (make gcc-5.1 the minimum version)"

- Sedat -

https://git.kernel.org/linus/316346243be6df12799c0b64b788e06bad97c30b

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOdmCS5Q7AzUL5nziYVU7RrtRjoE9JjOXfVBWagO1Bzbsew%40mail.gmail.com.

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 12:57 PM Sedat Dilek <sedat...@gmail.com> wrote:
>
> On Mon, Sep 13, 2021 at 9:53 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-bu...@googlegroups.com> wrote:
> >
> > On Mon, Sep 13, 2021 at 11:39 AM Nick Desaulniers
> > <ndesau...@google.com> wrote:
> > >
> > > There's an issue with my alternative approach
> > > (https://gist.github.com/nickdesaulniers/2479818f4983bbf2d688cebbab435863)
> > > with declaring the local variable z in div_64() since either operand
> > > could be 64b, which result in an unwanted truncation if the dividend
> > > is 32b (or less, and divisor is 64b). I think (what I realized this
> > > weekend) is that we might be able to replace the `if` with
> > > `__builtin_choose_expr`, then have that whole expression be the final
> > > statement and thus the "return value" of the statement expression.
> >
> > Christ...that...works? Though, did Linus just merge my patches for gcc 5.1?
> >
>
> "Merge branch 'gcc-min-version-5.1' (make gcc-5.1 the minimum version)"

Ha! I pulled+rebased and this code disappeared...I thought I had
rebased on the wrong branch or committed work to master accidentally.
Patch to stable-only inbound.
--
Thanks,
~Nick Desaulniers

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 1:10 PM Linus Torvalds
<torv...@linux-foundation.org> wrote:

>
> On Mon, Sep 13, 2021 at 1:02 PM Nick Desaulniers
> <ndesau...@google.com> wrote:
> >
> > Ha! I pulled+rebased and this code disappeared...I thought I had
> > rebased on the wrong branch or committed work to master accidentally.
> > Patch to stable-only inbound.
>

> Side note: for stable, can you look into using _Generic() instead of
> __builtin_choose_expression() with typeof, or some
> __builtin_types_compatible_p() magic?
>
> Yes, yes, we use __builtin_choose_expression() elsewhere, but we've
> started using _Generic(), and it's really the more natural model - in
> addition to being the standard C one.
>
> Of course, there may be some reason why _Generic() doesn't work, but
> it _is_ the natural fit for any "for type X, do Y" kind of thing.
>
> No?

Man, c'mon, I just got the __builtin_choose_expression() working! It's
not...too bad...ish. (Besides, I'd actually have to learn how to use
_Generic...I've never quite gotten anything I've written trying to use
it to actually compile).

Do we have access to _Generic in GCC 4.9?
--
Thanks,
~Nick Desaulniers

[Linus Torvalds]

On Mon, Sep 13, 2021 at 1:02 PM Nick Desaulniers
<ndesau...@google.com> wrote:
>

> Ha! I pulled+rebased and this code disappeared...I thought I had
> rebased on the wrong branch or committed work to master accidentally.
> Patch to stable-only inbound.

Side note: for stable, can you look into using _Generic() instead of
__builtin_choose_expression() with typeof, or some
__builtin_types_compatible_p() magic?

Yes, yes, we use __builtin_choose_expression() elsewhere, but we've
started using _Generic(), and it's really the more natural model - in
addition to being the standard C one.

Of course, there may be some reason why _Generic() doesn't work, but
it _is_ the natural fit for any "for type X, do Y" kind of thing.

No?

Linus

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 1:16 PM Nick Desaulniers

<ndesau...@google.com> wrote:
>
> On Mon, Sep 13, 2021 at 1:10 PM Linus Torvalds
> <torv...@linux-foundation.org> wrote:
> >
> > On Mon, Sep 13, 2021 at 1:02 PM Nick Desaulniers
> > <ndesau...@google.com> wrote:
> > >
> > > Ha! I pulled+rebased and this code disappeared...I thought I had
> > > rebased on the wrong branch or committed work to master accidentally.
> > > Patch to stable-only inbound.
> >
> > Side note: for stable, can you look into using _Generic() instead of
> > __builtin_choose_expression() with typeof, or some
> > __builtin_types_compatible_p() magic?
> >
> > Yes, yes, we use __builtin_choose_expression() elsewhere, but we've
> > started using _Generic(), and it's really the more natural model - in
> > addition to being the standard C one.
> >
> > Of course, there may be some reason why _Generic() doesn't work, but
> > it _is_ the natural fit for any "for type X, do Y" kind of thing.
> >
> > No?
>
> Man, c'mon, I just got the __builtin_choose_expression() working! It's
> not...too bad...ish. (Besides, I'd actually have to learn how to use
> _Generic...I've never quite gotten anything I've written trying to use
> it to actually compile).
>
> Do we have access to _Generic in GCC 4.9?

Follow up thread, sorry/not sorry for not taking the full cc list:
https://lore.kernel.org/stable/20210913203201.184...@google.com/
--
Thanks,
~Nick Desaulniers

[Linus Torvalds]

On Mon, Sep 13, 2021 at 1:16 PM Nick Desaulniers
<ndesau...@google.com> wrote:
>
> Do we have access to _Generic in GCC 4.9?

We've ended up using it unconditionally since last year, so yes.

In fact, the compiler version tests got removed when we raised the gcc
version requirement to 4.9 in commit 6ec4476ac825 ("Raise gcc version
requirement to 4.9"):

"In particular, raising the minimum to 4.9 means that we can now just
assume _Generic() exists, which is likely the much better replacement
for a lot of very convoluted built-time magic with conditionals on
sizeof and/or __builtin_choose_expr() with same_type() etc"

but we haven't used it much since.

The "seqprop" code for picking the right lock for seqlock is perhaps
the main example, and staring at that code will make you go blind, so
look away.

Linus

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 1:42 PM Linus Torvalds
<torv...@linux-foundation.org> wrote:
>
> On Mon, Sep 13, 2021 at 1:16 PM Nick Desaulniers
> <ndesau...@google.com> wrote:
> >
> > Do we have access to _Generic in GCC 4.9?
>
> We've ended up using it unconditionally since last year, so yes.

Sorry, grepping would have taken < 1s. I'm very lazy.
http://threevirtues.com/

>
> In fact, the compiler version tests got removed when we raised the gcc
> version requirement to 4.9 in commit 6ec4476ac825 ("Raise gcc version
> requirement to 4.9"):
>
> "In particular, raising the minimum to 4.9 means that we can now just
> assume _Generic() exists, which is likely the much better replacement
> for a lot of very convoluted built-time magic with conditionals on
> sizeof and/or __builtin_choose_expr() with same_type() etc"
>
> but we haven't used it much since.
>
> The "seqprop" code for picking the right lock for seqlock is perhaps
> the main example, and staring at that code will make you go blind, so
> look away.

Looking at my patch:
https://lore.kernel.org/stable/20210913203201.184...@google.com/
I don't think _Generic helps us in the case of dispatching based on
the result of is_signed_type() (the operands could undergo type
promotion, so we'd need lots of cases that are more concisely covered
by is_signed_type()). It could replace the nested checks in div_64
with nested _Generics, I think. Not sure it's a huge win for
readability. Maybe cut the number of expansions of the parameters in
half though. Let me give it a try just to see what it looks like.
--
Thanks,
~Nick Desaulniers

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 1:50 PM Nick Desaulniers

Is this more readable? Same line count. I'm not sure if there's such
a thing as "fallthrough" between the "cases" of _Generic to minimize
duplication, or whether this could be factored further. Needs lots
more () around macro param use and tab'ed out line endings...






diff --git a/include/linux/math64.h b/include/linux/math64.h
index 8652a8a35d70..8fc4d56a45b9 100644
--- a/include/linux/math64.h
+++ b/include/linux/math64.h
@@ -162,17 +162,17 @@ static inline s64 div_s64(s64 dividend, s32 divisor)
div_u64(dividend, divisor)); \
})

-#define __div_64(dividend) _Generic((divisor), \
- s64: div64_x64, \
- u64: div64_x64, \
- default: div_x64)
+#define __div_64(dividend, divisor) _Generic((divisor), \
+ s64: div64_x64(dividend, divisor), \
+ u64: div64_x64(dividend, divisor), \
+ default: div_x64(dividend, divisor))

#define div_64(dividend, divisor) ({
\
BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u64),
\
"128b div unsupported");
\
_Generic((dividend), \
- s64: __div_64(dividend)(dividend, divisor), \
- u64: __div_64(dividend)(dividend, divisor), \
+ s64: __div_64(dividend, divisor), \
+ u64: __div_64(dividend, divisor), \
default: dividend / divisor); \
})

--
Thanks,
~Nick Desaulniers

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 2:13 PM Nick Desaulniers

Sorry wrong diff:
diff --git a/include/linux/math64.h b/include/linux/math64.h
index bc9c12c168d0..8fc4d56a45b9 100644
--- a/include/linux/math64.h
+++ b/include/linux/math64.h
@@ -162,18 +162,18 @@ static inline s64 div_s64(s64 dividend, s32 divisor)
div_u64(dividend, divisor)); \
})

+#define __div_64(dividend, divisor) _Generic((divisor), \
+ s64: div64_x64(dividend, divisor), \
+ u64: div64_x64(dividend, divisor), \
+ default: div_x64(dividend, divisor))

+

#define div_64(dividend, divisor) ({
\
BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u64),
\
"128b div unsupported");
\

- __builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(dividend), s64) ||
\
- __builtin_types_compatible_p(typeof(dividend), u64),
\
- __builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(divisor),
s64) || \
- __builtin_types_compatible_p(typeof(divisor),
u64), \
- div64_x64(dividend, divisor),
\
- div_x64(dividend, divisor)),
\
- dividend / divisor);
\
+ _Generic((dividend), \

+ s64: __div_64(dividend, divisor), \
+ u64: __div_64(dividend, divisor), \

+ default: dividend / divisor); \

[Linus Torvalds]

On Mon, Sep 13, 2021 at 2:15 PM Nick Desaulniers
<ndesau...@google.com> wrote:
>
> Sorry wrong diff:

Well, this second diff was seriously whitespace-damaged and hard to
read, but while it seems to be the same number of lines, it sure looks
a lot more readable in this format.

Except I think that

default: dividend / divisor);

should really have parentheses around both of those macro arguments.

That's a preexisting problem, but it should be fixed while at it.

I'm also not sure why that (again, preexisting) BUILD_BUG_ON_MSG()
only checks the size of the dividend, not the divisor. Very strange.
But probably not worth worrying about.

Linus

[Nick Desaulniers]

On Mon, Sep 13, 2021 at 4:00 PM Linus Torvalds
<torv...@linux-foundation.org> wrote:
>
> On Mon, Sep 13, 2021 at 2:15 PM Nick Desaulniers
> <ndesau...@google.com> wrote:
> >
> > Sorry wrong diff:
>
> Well, this second diff was seriously whitespace-damaged and hard to
> read, but while it seems to be the same number of lines, it sure looks
> a lot more readable in this format.
>
> Except I think that
>
> default: dividend / divisor);
>
> should really have parentheses around both of those macro arguments.
>
> That's a preexisting problem, but it should be fixed while at it.

Ok, I'll send a revised v2 based on _Generic; Rasmus can help review
when he's awake.

> I'm also not sure why that (again, preexisting) BUILD_BUG_ON_MSG()
> only checks the size of the dividend, not the divisor. Very strange.
> But probably not worth worrying about.

I sent a not-yet-applied diff of my not-yet-applied diff. I was
playing with this last week, and IIRC we had divisors that were less
than 32b being promoted to int. But I'll test it some more.
--
Thanks,
~Nick Desaulniers

[libaokun (A)]

How about deleting the check_mul_overflow in the __nbd_ioctl as follows?

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5170a630778d..f404e0540476 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1393,7 +1393,6 @@ static int __nbd_ioctl(struct block_device *bdev,

struct nbd_device *nbd,
                       unsigned int cmd, unsigned long arg)
 {
        struct nbd_config *config = nbd->config;

-       loff_t bytesize;

        switch (cmd) {
        case NBD_DISCONNECT:
@@ -1408,9 +1407,10 @@ static int __nbd_ioctl(struct block_device *bdev,

struct nbd_device *nbd,
        case NBD_SET_SIZE:
                return nbd_set_size(nbd, arg, config->blksize);
        case NBD_SET_SIZE_BLOCKS:

-               if (check_mul_overflow((loff_t)arg, config->blksize,
&bytesize))
+               if (arg && (LLONG_MAX / arg <= config->blksize))
                        return -EINVAL;
-               return nbd_set_size(nbd, bytesize, config->blksize);
+               return nbd_set_size(nbd, arg * config->blksize,
+                                   config->blksize);

        case NBD_SET_TIMEOUT:
                nbd_set_cmd_timeout(nbd, arg);
                return 0;

--
2.31.1

--
With Best Regards,
Baokun Li

[Nick Desaulniers]

64b division is going to need do_div(), yeah? Besides, this is likely
to pop up again for other callers of check_mul_overflow(), might as
well fix it.

> - return nbd_set_size(nbd, bytesize, config->blksize);
> + return nbd_set_size(nbd, arg * config->blksize,
> + config->blksize);
> case NBD_SET_TIMEOUT:
> nbd_set_cmd_timeout(nbd, arg);
> return 0;
> --
> 2.31.1
>
> --
> With Best Regards,
> Baokun Li
>
>
>

--
Thanks,
~Nick Desaulniers

[David Laight]

...

>         case NBD_SET_SIZE:
>                 return nbd_set_size(nbd, arg, config->blksize);
>         case NBD_SET_SIZE_BLOCKS:
> -               if (check_mul_overflow((loff_t)arg, config->blksize,
> &bytesize))
> +               if (arg && (LLONG_MAX / arg <= config->blksize))
>                         return -EINVAL;
> -               return nbd_set_size(nbd, bytesize, config->blksize);
> +               return nbd_set_size(nbd, arg * config->blksize,
> +                                   config->blksize);

Shouldn't there just be sanity bound checks on 'config->blksize' and
'arg' so that the product is never going to overflow?

It isn't as though any values near the overflow limit are sane.

I suspect you could check config->blksize <= 64k && arg <= 32k
and even that would be generous.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)