linux-kernel - Re: [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Jul 2019 10:40:40 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Tejun Heo <tj@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Jeffrin Thalakkottoor <jeffrin@...agiritech.edu.in>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        tobin@...nel.org, Dmitry Vyukov <dvyukov@...gle.com>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH] libata: zpodd: Fix small read overflow in zpodd_get_mech_type()

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