* [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type()
@ 2019-07-20 3:41 Kees Cook
2019-07-22 17:40 ` Nick Desaulniers
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2019-07-20 3:41 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, Dan Carpenter, Jeffrin Thalakkottoor,
Nick Desaulniers, Steven Rostedt, Andy Shevchenko,
Alexander Shishkin, tobin, Dmitry Vyukov, Alexander Potapenko
Much like commit 18c9a99bce2a ("libata: zpodd: small read overflow in
eject_tray()"), this fixes a cdb[] buffer length, this time in
zpodd_get_mech_type():
We read from the cdb[] buffer in ata_exec_internal_sg(). It has to be
ATAPI_CDB_LEN (16) bytes long, but this buffer is only 12 bytes.
The KASAN report was:
BUG: KASAN: global-out-of-bounds in ata_exec_internal_sg+0x50f/0xc70
Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
...
The buggy address belongs to the variable:
cdb.48319+0x0/0x40
Reported-by: Jeffrin Thalakkottoor <jeffrin@rajagiritech.edu.in>
Fixes: afe759511808c ("libata: identify and init ZPODD devices")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/ata/libata-zpodd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 */
--
2.17.1
--
Kees Cook
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type()
2019-07-20 3:41 [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type() Kees Cook
@ 2019-07-22 17:40 ` Nick Desaulniers
2019-07-22 18:18 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Nick Desaulniers @ 2019-07-22 17:40 UTC (permalink / raw)
To: Kees Cook
Cc: Tejun Heo, LKML, Dan Carpenter, Jeffrin Thalakkottoor,
Steven Rostedt, Andy Shevchenko, Alexander Shishkin, tobin,
Dmitry Vyukov, Alexander Potapenko
On Fri, Jul 19, 2019 at 8:41 PM Kees Cook <keescook@chromium.org> wrote:
>
> Much like commit 18c9a99bce2a ("libata: zpodd: small read overflow in
> eject_tray()"), this fixes a cdb[] buffer length, this time in
> zpodd_get_mech_type():
>
> We read from the cdb[] buffer in ata_exec_internal_sg(). It has to be
> ATAPI_CDB_LEN (16) bytes long, but this buffer is only 12 bytes.
>
> The KASAN report was:
>
> BUG: KASAN: global-out-of-bounds in ata_exec_internal_sg+0x50f/0xc70
> Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
> ...
> The buggy address belongs to the variable:
> cdb.48319+0x0/0x40
>
> Reported-by: Jeffrin Thalakkottoor <jeffrin@rajagiritech.edu.in>
> Fixes: afe759511808c ("libata: identify and init ZPODD devices")
> Signed-off-by: Kees Cook <keescook@chromium.org>
Interesting, both initializer lists are less than ATAPI_CDB_LEN (16)
elements, yet ata_exec_internal_sg() in drivers/ata/libata-core.c very
clearly has a ATAPI_CDB_LEN byte memcpy on that element. Probably
both initializer lists should just lead off the trailing zero's or
provide the entire 16 elements. For now, thank you for the patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> drivers/ata/libata-zpodd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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 */
> --
> 2.17.1
>
>
> --
> Kees Cook
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type()
2019-07-22 17:40 ` Nick Desaulniers
@ 2019-07-22 18:18 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2019-07-22 18:18 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Tejun Heo, LKML, Dan Carpenter, Jeffrin Thalakkottoor,
Steven Rostedt, Andy Shevchenko, Alexander Shishkin, tobin,
Dmitry Vyukov, Alexander Potapenko
On Mon, Jul 22, 2019 at 10:40:40AM -0700, Nick Desaulniers wrote:
> On Fri, Jul 19, 2019 at 8:41 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Much like commit 18c9a99bce2a ("libata: zpodd: small read overflow in
> > eject_tray()"), this fixes a cdb[] buffer length, this time in
> > zpodd_get_mech_type():
> >
> > We read from the cdb[] buffer in ata_exec_internal_sg(). It has to be
> > ATAPI_CDB_LEN (16) bytes long, but this buffer is only 12 bytes.
> >
> > The KASAN report was:
> >
> > BUG: KASAN: global-out-of-bounds in ata_exec_internal_sg+0x50f/0xc70
> > Read of size 16 at addr ffffffff91f41f80 by task scsi_eh_1/149
> > ...
> > The buggy address belongs to the variable:
> > cdb.48319+0x0/0x40
> >
> > Reported-by: Jeffrin Thalakkottoor <jeffrin@rajagiritech.edu.in>
> > Fixes: afe759511808c ("libata: identify and init ZPODD devices")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Interesting, both initializer lists are less than ATAPI_CDB_LEN (16)
> elements, yet ata_exec_internal_sg() in drivers/ata/libata-core.c very
> clearly has a ATAPI_CDB_LEN byte memcpy on that element. Probably
> both initializer lists should just lead off the trailing zero's or
> provide the entire 16 elements. For now, thank you for the patch.
Yup, with the specific size added it'll be zero-padded.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Thanks!
-Kees
>
> > ---
> > drivers/ata/libata-zpodd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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 */
> > --
> > 2.17.1
> >
> >
> > --
> > Kees Cook
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-22 18:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-20 3:41 [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type() Kees Cook
2019-07-22 17:40 ` Nick Desaulniers
2019-07-22 18:18 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).