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] [day] [month] [year] [list]

Message-ID: <201907221117.254D219F9E@keescook>
Date:   Mon, 22 Jul 2019 11:18:12 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
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 Mon, Jul 22, 2019 at 10:40:40AM -0700, Nick Desaulniers wrote:
> 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.

Yup, with the specific size added it'll be zero-padded.

> Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.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