hello all ,
i encountered a KASAN bug related . here are some related information...
-------------------x-----------------------------x------------------
[ 30.037312] BUG: KASAN: global-out-of-bounds in
ata_exec_internal_sg+0x50f/0xc70
[ 30.037447] Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
[ 30.039935] The buggy address belongs to the variable:
[ 30.040059] cdb.48319+0x0/0x40
[ 30.040241] Memory state around the buggy address:
[ 30.040362] ffffffff91f41e80: fa fa fa fa 00 00 fa fa fa fa fa fa
00 00 07 fa
[ 30.040498] ffffffff91f41f00: fa fa fa fa 00 00 00 00 00 00 00 03
fa fa fa fa
[ 30.040628] >ffffffff91f41f80: 00 04 fa fa fa fa fa fa 00 00 fa fa
fa fa fa fa
[ 30.040755] ^
[ 30.040868] ffffffff91f42000: 00 00 00 04 fa fa fa fa 00 fa fa fa
fa fa fa fa
[ 30.041003] ffffffff91f42080: 04 fa fa fa fa fa fa fa 00 04 fa fa
fa fa fa fa
---------------------------x--------------------------x----------------
$uname -a
Linux debian 5.2.0-rc7+ #4 SMP Tue Jul 9 02:54:07 IST 2019 x86_64 GNU/Linux
$
--------------------x----------------------------x---------------------------
(gdb) l *ata_exec_internal_sg+0x50f
0xffffffff81c7b59f is in ata_exec_internal_sg (./include/linux/string.h:359).
354 if (q_size < size)
355 __read_overflow2();
356 }
357 if (p_size < size || q_size < size)
358 fortify_panic(__func__);
359 return __builtin_memcpy(p, q, size);
360 }
361
362 __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
363 {
(gdb)
--------------------------x--------------------------
GNU Make 4.2.1
Binutils 2.31.1
Util-linux 2.33.1
Mount 2.33.1
Linux C Library 2.28
Dynamic linker (ldd) 2.28
Procps 3.3.15
Kbd 2.0.4
Console-tools 2.0.4
Sh-utils 8.30
Udev 241
---------------------x--------------------------------x
Thread model: posix
gcc version 8.3.0 (Debian 8.3.0-7)
---------------------x--------------------------------x
Please ask if more information is needed.
--
software engineer
rajagiri school of engineering and technology
On Wed, Jul 10, 2019 at 10:44 AM Jeffrin Thalakkottoor
<[email protected]> wrote:
>
> hello all ,
>
> i encountered a KASAN bug related . here are some related information...
>
>
> -------------------x-----------------------------x------------------
> [ 30.037312] BUG: KASAN: global-out-of-bounds in
> ata_exec_internal_sg+0x50f/0xc70
> [ 30.037447] Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
>
>
> [ 30.039935] The buggy address belongs to the variable:
> [ 30.040059] cdb.48319+0x0/0x40
>
> [ 30.040241] Memory state around the buggy address:
> [ 30.040362] ffffffff91f41e80: fa fa fa fa 00 00 fa fa fa fa fa fa
> 00 00 07 fa
> [ 30.040498] ffffffff91f41f00: fa fa fa fa 00 00 00 00 00 00 00 03
> fa fa fa fa
> [ 30.040628] >ffffffff91f41f80: 00 04 fa fa fa fa fa fa 00 00 fa fa
> fa fa fa fa
> [ 30.040755] ^
> [ 30.040868] ffffffff91f42000: 00 00 00 04 fa fa fa fa 00 fa fa fa
> fa fa fa fa
> [ 30.041003] ffffffff91f42080: 04 fa fa fa fa fa fa fa 00 04 fa fa
> fa fa fa fa
>
> ---------------------------x--------------------------x----------------
> $uname -a
> Linux debian 5.2.0-rc7+ #4 SMP Tue Jul 9 02:54:07 IST 2019 x86_64 GNU/Linux
> $
>
> --------------------x----------------------------x---------------------------
> (gdb) l *ata_exec_internal_sg+0x50f
> 0xffffffff81c7b59f is in ata_exec_internal_sg (./include/linux/string.h:359).
So looks like ata_exec_internal_sg() is panic'ing when...
> 354 if (q_size < size)
> 355 __read_overflow2();
> 356 }
> 357 if (p_size < size || q_size < size)
> 358 fortify_panic(__func__);
> 359 return __builtin_memcpy(p, q, size);
> 360 }
> 361
> 362 __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
...a call to memmove is made? Without having looked at the source of
ata_exec_internal_sg(), it's possible that either through inlining, or
the compiler generating a memmove, that one of the arguments was not
quite right. I suggest spending more time isolating where this is
coming from, if you can reliably reproduce, or CC whoever wrote or
maintains the code and ask them to take a look.
The cited code looks like a check comparing that the pointer distance
is greater than the size of bytes being passed in. I'd wager
someone's calling memmove with overlapping memory regions when they
really wanted memcpy. Maybe a better question, is why was memmove
ever used; if there was some invariant that the memory regions
overlapped, why is that invariant no longer holding.
Anyways, sorry I don't have more time to look into this. Thank you
for the report.
> 363 {
> (gdb)
> --------------------------x--------------------------
> GNU Make 4.2.1
> Binutils 2.31.1
> Util-linux 2.33.1
> Mount 2.33.1
> Linux C Library 2.28
> Dynamic linker (ldd) 2.28
> Procps 3.3.15
> Kbd 2.0.4
> Console-tools 2.0.4
> Sh-utils 8.30
> Udev 241
> ---------------------x--------------------------------x
> Thread model: posix
> gcc version 8.3.0 (Debian 8.3.0-7)
> ---------------------x--------------------------------x
>
> Please ask if more information is needed.
>
> --
> software engineer
> rajagiri school of engineering and technology
--
Thanks,
~Nick Desaulniers
On Tue, 16 Jul 2019 11:28:29 -0700
Nick Desaulniers <[email protected]> wrote:
> The cited code looks like a check comparing that the pointer distance
> is greater than the size of bytes being passed in. I'd wager
> someone's calling memmove with overlapping memory regions when they
> really wanted memcpy. Maybe a better question, is why was memmove
> ever used; if there was some invariant that the memory regions
> overlapped, why is that invariant no longer holding.
I'm confused by the above statement as memmove() allows overlapping of
src and dest, where as memcpy() does not.
-- Steve
On Tue, Jul 16, 2019 at 11:57 AM Steven Rostedt <[email protected]> wrote:
>
> On Tue, 16 Jul 2019 11:28:29 -0700
> Nick Desaulniers <[email protected]> wrote:
>
> > The cited code looks like a check comparing that the pointer distance
> > is greater than the size of bytes being passed in. I'd wager
> > someone's calling memmove with overlapping memory regions when they
> > really wanted memcpy. Maybe a better question, is why was memmove
> > ever used; if there was some invariant that the memory regions
> > overlapped, why is that invariant no longer holding.
>
> I'm confused by the above statement as memmove() allows overlapping of
> src and dest, where as memcpy() does not.
Yes you're right; I confused the two. From the snippet in the
original email, it looks like the body of a fortified memcpy was
provided, and a memmove declaration was below it. So replace my
assumption of a bad call to memmove with a bad call to memcpy (which
should then make more sense, hopefully).
--
Thanks,
~Nick Desaulniers
On Tue, Jul 16, 2019 at 11:28:29AM -0700, Nick Desaulniers wrote:
> On Wed, Jul 10, 2019 at 10:44 AM Jeffrin Thalakkottoor
> <[email protected]> wrote:
> >
> > hello all ,
> >
> > i encountered a KASAN bug related . here are some related information...
> >
> >
> > -------------------x-----------------------------x------------------
> > [ 30.037312] BUG: KASAN: global-out-of-bounds in
> > ata_exec_internal_sg+0x50f/0xc70
> > [ 30.037447] Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
> >
> >
> > [ 30.039935] The buggy address belongs to the variable:
> > [ 30.040059] cdb.48319+0x0/0x40
> > (gdb) l *ata_exec_internal_sg+0x50f
> > 0xffffffff81c7b59f is in ata_exec_internal_sg (./include/linux/string.h:359).
>
> So looks like ata_exec_internal_sg() is panic'ing when...
>
> > 354 if (q_size < size)
> > 355 __read_overflow2();
> > 356 }
> > 357 if (p_size < size || q_size < size)
> > 358 fortify_panic(__func__);
> > 359 return __builtin_memcpy(p, q, size);
^^^ here, so within memcpy(), but after the "easy" sanity checks.
The only place where I see ata_exec_internal_sg() calling memcpy() is
here:
/* prepare & issue qc */
qc->tf = *tf;
if (cdb)
memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
the "16" is consistent with the report:
include/linux/ata.h: ATAPI_CDB_LEN = 16,
which matches the claim about the cdb variable from KASAN. And it's a
read, so "cdb" is wrong. Do you have a longer back trace? What called
ata_exec_internal_sg()?
ata_exec_internal() is the only caller of ata_exec_internal_sg(). Nearly
all callers of ata_exec_internal() pass a NULL cdb. Those that don't
are:
atapi_eh_tur()
u8 cdb[ATAPI_CDB_LEN] = ...
atapi_eh_request_sense()
u8 cdb[ATAPI_CDB_LEN] = ...
These two are on the static and correctly sized.
eject_tray()
static const char cdb[ATAPI_CDB_LEN] = ...
zpodd_get_mech_type()
static const char cdb[] = ...
These are both in rodata, and only the first is correctly sized. I
assume the following will fix it:
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 173e6f2dd9af..eefda51f97d3 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -56,7 +56,7 @@ static enum odd_mech_type zpodd_get_mech_type(struct ata_device *dev)
unsigned int ret;
struct rm_feature_desc *desc;
struct ata_taskfile tf;
- static const char cdb[] = { GPCMD_GET_CONFIGURATION,
+ static const char cdb[ATAPI_CDB_LEN] = { GPCMD_GET_CONFIGURATION,
2, /* only 1 feature descriptor requested */
0, 3, /* 3, removable medium feature */
0, 0, 0,/* reserved */
--
Kees Cook
hello Jens Axboe,
Please can you take a look at related code and also patch from Kees ?
On Tue, Jul 16, 2019 at 11:58 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Wed, Jul 10, 2019 at 10:44 AM Jeffrin Thalakkottoor
> <[email protected]> wrote:
> >
> > hello all ,
> >
> > i encountered a KASAN bug related . here are some related information...
> >
> >
> > -------------------x-----------------------------x------------------
> > [ 30.037312] BUG: KASAN: global-out-of-bounds in
> > ata_exec_internal_sg+0x50f/0xc70
> > [ 30.037447] Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
> >
> >
> > [ 30.039935] The buggy address belongs to the variable:
> > [ 30.040059] cdb.48319+0x0/0x40
> >
> > [ 30.040241] Memory state around the buggy address:
> > [ 30.040362] ffffffff91f41e80: fa fa fa fa 00 00 fa fa fa fa fa fa
> > 00 00 07 fa
> > [ 30.040498] ffffffff91f41f00: fa fa fa fa 00 00 00 00 00 00 00 03
> > fa fa fa fa
> > [ 30.040628] >ffffffff91f41f80: 00 04 fa fa fa fa fa fa 00 00 fa fa
> > fa fa fa fa
> > [ 30.040755] ^
> > [ 30.040868] ffffffff91f42000: 00 00 00 04 fa fa fa fa 00 fa fa fa
> > fa fa fa fa
> > [ 30.041003] ffffffff91f42080: 04 fa fa fa fa fa fa fa 00 04 fa fa
> > fa fa fa fa
> >
> > ---------------------------x--------------------------x----------------
> > $uname -a
> > Linux debian 5.2.0-rc7+ #4 SMP Tue Jul 9 02:54:07 IST 2019 x86_64 GNU/Linux
> > $
> >
> > --------------------x----------------------------x---------------------------
> > (gdb) l *ata_exec_internal_sg+0x50f
> > 0xffffffff81c7b59f is in ata_exec_internal_sg (./include/linux/string.h:359).
>
> So looks like ata_exec_internal_sg() is panic'ing when...
>
> > 354 if (q_size < size)
> > 355 __read_overflow2();
> > 356 }
> > 357 if (p_size < size || q_size < size)
> > 358 fortify_panic(__func__);
> > 359 return __builtin_memcpy(p, q, size);
> > 360 }
> > 361
> > 362 __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
>
> ...a call to memmove is made? Without having looked at the source of
> ata_exec_internal_sg(), it's possible that either through inlining, or
> the compiler generating a memmove, that one of the arguments was not
> quite right. I suggest spending more time isolating where this is
> coming from, if you can reliably reproduce, or CC whoever wrote or
> maintains the code and ask them to take a look.
>
> The cited code looks like a check comparing that the pointer distance
> is greater than the size of bytes being passed in. I'd wager
> someone's calling memmove with overlapping memory regions when they
> really wanted memcpy. Maybe a better question, is why was memmove
> ever used; if there was some invariant that the memory regions
> overlapped, why is that invariant no longer holding.
>
> Anyways, sorry I don't have more time to look into this. Thank you
> for the report.
>
> > 363 {
> > (gdb)
> > --------------------------x--------------------------
> > GNU Make 4.2.1
> > Binutils 2.31.1
> > Util-linux 2.33.1
> > Mount 2.33.1
> > Linux C Library 2.28
> > Dynamic linker (ldd) 2.28
> > Procps 3.3.15
> > Kbd 2.0.4
> > Console-tools 2.0.4
> > Sh-utils 8.30
> > Udev 241
> > ---------------------x--------------------------------x
> > Thread model: posix
> > gcc version 8.3.0 (Debian 8.3.0-7)
> > ---------------------x--------------------------------x
> >
> > Please ask if more information is needed.
> >
> > --
> > software engineer
> > rajagiri school of engineering and technology
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
software engineer
rajagiri school of engineering and technology
On 7/29/19 1:34 PM, Jeffrin Thalakkottoor wrote:
> hello Kees Cook,
>
> i tested your fix and i think it worked like a charm !
> kasan message related disappeared during boot time and it does not
> show in the output of "sudo dmesg -l err"
> anyway thanks a lot !
Kees, could you send that out as a proper patch?
--
Jens Axboe
hello Kees Cook,
i tested your fix and i think it worked like a charm !
kasan message related disappeared during boot time and it does not
show in the output of "sudo dmesg -l err"
anyway thanks a lot !
On Fri, Jul 19, 2019 at 3:05 AM Kees Cook <[email protected]> wrote:
>
> On Tue, Jul 16, 2019 at 11:28:29AM -0700, Nick Desaulniers wrote:
> > On Wed, Jul 10, 2019 at 10:44 AM Jeffrin Thalakkottoor
> > <[email protected]> wrote:
> > >
> > > hello all ,
> > >
> > > i encountered a KASAN bug related . here are some related information...
> > >
> > >
> > > -------------------x-----------------------------x------------------
> > > [ 30.037312] BUG: KASAN: global-out-of-bounds in
> > > ata_exec_internal_sg+0x50f/0xc70
> > > [ 30.037447] Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
> > >
> > >
> > > [ 30.039935] The buggy address belongs to the variable:
> > > [ 30.040059] cdb.48319+0x0/0x40
> > > (gdb) l *ata_exec_internal_sg+0x50f
> > > 0xffffffff81c7b59f is in ata_exec_internal_sg (./include/linux/string.h:359).
> >
> > So looks like ata_exec_internal_sg() is panic'ing when...
> >
> > > 354 if (q_size < size)
> > > 355 __read_overflow2();
> > > 356 }
> > > 357 if (p_size < size || q_size < size)
> > > 358 fortify_panic(__func__);
> > > 359 return __builtin_memcpy(p, q, size);
>
> ^^^ here, so within memcpy(), but after the "easy" sanity checks.
>
> The only place where I see ata_exec_internal_sg() calling memcpy() is
> here:
>
> /* prepare & issue qc */
> qc->tf = *tf;
> if (cdb)
> memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
>
> the "16" is consistent with the report:
>
> include/linux/ata.h: ATAPI_CDB_LEN = 16,
>
> which matches the claim about the cdb variable from KASAN. And it's a
> read, so "cdb" is wrong. Do you have a longer back trace? What called
> ata_exec_internal_sg()?
>
> ata_exec_internal() is the only caller of ata_exec_internal_sg(). Nearly
> all callers of ata_exec_internal() pass a NULL cdb. Those that don't
> are:
>
> atapi_eh_tur()
> u8 cdb[ATAPI_CDB_LEN] = ...
> atapi_eh_request_sense()
> u8 cdb[ATAPI_CDB_LEN] = ...
>
> These two are on the static and correctly sized.
>
> eject_tray()
> static const char cdb[ATAPI_CDB_LEN] = ...
> zpodd_get_mech_type()
> static const char cdb[] = ...
>
> These are both in rodata, and only the first is correctly sized. I
> assume the following will fix it:
>
>
> diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
> index 173e6f2dd9af..eefda51f97d3 100644
> --- a/drivers/ata/libata-zpodd.c
> +++ b/drivers/ata/libata-zpodd.c
> @@ -56,7 +56,7 @@ static enum odd_mech_type zpodd_get_mech_type(struct ata_device *dev)
> unsigned int ret;
> struct rm_feature_desc *desc;
> struct ata_taskfile tf;
> - static const char cdb[] = { GPCMD_GET_CONFIGURATION,
> + static const char cdb[ATAPI_CDB_LEN] = { GPCMD_GET_CONFIGURATION,
> 2, /* only 1 feature descriptor requested */
> 0, 3, /* 3, removable medium feature */
> 0, 0, 0,/* reserved */
>
>
>
> --
> Kees Cook
--
software engineer
rajagiri school of engineering and technology
hello Kees,
please mention me ...
Reported-by: Jeffrin Jose T <[email protected]>
Tested-by: Jeffrin Jose T <[email protected]>
On Tue, Jul 30, 2019 at 1:25 AM Jens Axboe <[email protected]> wrote:
>
> On 7/29/19 1:34 PM, Jeffrin Thalakkottoor wrote:
> > hello Kees Cook,
> >
> > i tested your fix and i think it worked like a charm !
> > kasan message related disappeared during boot time and it does not
> > show in the output of "sudo dmesg -l err"
> > anyway thanks a lot !
>
> Kees, could you send that out as a proper patch?
>
> --
> Jens Axboe
>
--
software engineer
rajagiri school of engineering and technology