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

[Kees Cook <keescook@chromium.org>]

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