Re: [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type()

From: Nick Desaulniers
Date: Mon Jul 22 2019 - 13:40:57 EST


On Fri, Jul 19, 2019 at 8:41 PM Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> Fixes: afe759511808c ("libata: identify and init ZPODD devices")
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>

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@xxxxxxxxxx>

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