libata: zpodd: Fix small read overflow in zpodd_get_mech_type()
diff mbox series

Message ID 201907192038.AEF9B52@keescook
State In Next
Commit 71d6c505b4d9e6f76586350450e785e3d452b346
Headers show
Series
  • libata: zpodd: Fix small read overflow in zpodd_get_mech_type()
Related show

Commit Message

Kees Cook July 20, 2019, 3:41 a.m. UTC
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(-)

Comments

Nick Desaulniers July 22, 2019, 5:40 p.m. UTC | #1
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
Kees Cook July 22, 2019, 6:18 p.m. UTC | #2
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

Patch
diff mbox series

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 */