BUG: KASAN: global-out-of-bounds in ata_exec_internal_sg+0x50f/0xc70

DISCUSSION Jeffrin Thalakkottoor 5 participants 9 messages Jul 10, 2019 – Jul 29, 2019 libata

lore

Share & tools

lore

Jeffrin Thalakkottoor

author

2019-07-10 17:44 UTC #0 lore raw

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.

Signature · 2 lines

software engineer
rajagiri school of engineering and technology

Nick Desaulniers

2019-07-16 18:28 UTC #1 lore raw

Quoted 28 lines · Jeffrin Thalakkottoor

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

Quoted 8 lines

---------------------------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...

Quoted 9 lines

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.

Quoted 24 lines

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

Signature · 2 lines

Thanks,
~Nick Desaulniers

Steven Rostedt

2019-07-16 18:57 UTC #2 lore raw

Quoted 6 lines · On Tue, 16 Jul 2019 11:28:29 -0700 Nick Desaulniers <[email protected]>…

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

Nick Desaulniers

2019-07-16 19:45 UTC #3 lore raw

Quoted 11 lines · Steven Rostedt

Quoted 6 lines · On Tue, 16 Jul 2019 11:28:29 -0700 Nick Desaulniers <[email protected]>…
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.

Quoted 2 lines

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).

Signature · 2 lines

Thanks,
~Nick Desaulniers

Kees Cook

2019-07-18 21:35 UTC #4 lore raw

In reply to Nick Desaulniers

Quoted 14 lines · Nick Desaulniers

Quoted 12 lines · Jeffrin Thalakkottoor
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

Quoted 13 lines

Quoted 4 lines
[   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...
Quoted 6 lines
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:

drivers/ata/libata-zpodd.c +1 -1

index 173e6f2dd9af..eefda51f97d3 100644
@@ -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 */

Signature · 1 line

Kees Cook

Jeffrin Thalakkottoor

author

2019-07-29 19:35 UTC #5 lore raw

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 !

Quoted 86 lines · Kees Cook

Quoted 14 lines · Nick Desaulniers
Quoted 12 lines · Jeffrin Thalakkottoor
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
Quoted 13 lines
Quoted 4 lines
[   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...
Quoted 6 lines
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:
drivers/ata/libata-zpodd.c +1 -2
index 173e6f2dd9af..eefda51f97d3 100644
@@ -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

Signature · 2 lines

software engineer
rajagiri school of engineering and technology

Jens Axboe

2019-07-29 19:55 UTC #6 lore raw

Quoted 6 lines · Jeffrin Thalakkottoor

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?

Signature · 1 line

Jens Axboe

Jeffrin Thalakkottoor

author Tested-by

2019-07-29 20:14 UTC #7 lore raw

hello Kees,

please mention me ...

Reported-by Jeffrin Jose T <[email protected]>

Tested-by Jeffrin Jose T <[email protected]>

Quoted 14 lines · Jens Axboe

Quoted 6 lines · Jeffrin Thalakkottoor
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

Signature · 2 lines

software engineer
rajagiri school of engineering and technology

Jeffrin Thalakkottoor

author

2019-07-25 21:34 UTC #8 lore raw

In reply to Nick Desaulniers

hello Jens Axboe,

Please can you take a look at related code and also patch from Kees ?

Quoted 99 lines · Nick Desaulniers

Quoted 28 lines · Jeffrin Thalakkottoor
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
Quoted 8 lines
---------------------------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...
Quoted 9 lines
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.
Quoted 19 lines
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
Quoted 5 lines
Please ask if more information is needed.
--
software engineer
rajagiri school of engineering and technology
--
Thanks,
~Nick Desaulniers

Signature · 2 lines

software engineer
rajagiri school of engineering and technology