Re: sdmmc(4) off-by-one on boundary check

[Marcus Glocker]

On Tue, Mar 23, 2021 at 08:29:06AM -0600, Theo de Raadt wrote:

> Marcus Glocker <mar...@nazgul.ch> wrote:
> 
> > Index: sys/dev/sdmmc/sdmmc_scsi.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_scsi.c,v
> > retrieving revision 1.59
> > diff -u -p -u -p -r1.59 sdmmc_scsi.c
> > --- sys/dev/sdmmc/sdmmc_scsi.c      15 Oct 2020 13:22:13 -0000      1.59
> > +++ sys/dev/sdmmc/sdmmc_scsi.c      23 Mar 2021 07:32:52 -0000
> > @@ -365,9 +365,8 @@ sdmmc_scsi_cmd(struct scsi_xfer *xs)
> >     /* A read or write operation. */
> >     sdmmc_scsi_decode_rw(xs, &blockno, &blockcnt);
> >  
> > -   if (blockno >= tgt->card->csd.capacity ||
> > -       blockno + blockcnt > tgt->card->csd.capacity) {
> > -           DPRINTF(("%s: out of bounds %u-%u >= %u\n", DEVNAME(sc),
> > +   if (blockno + blockcnt >= tgt->card->csd.capacity) {
> > +           DPRINTF(("%s: out of bounds %u+%u >= %u\n", DEVNAME(sc),
> >                 blockno, blockcnt, tgt->card->csd.capacity));
> >             xs->error = XS_DRIVER_STUFFUP;
> >             scsi_done(xs);
> 
> blockno and blockcnt are both 32-bit, so this feels dangerously close
> to integer wrap, and I believe the first condition cannot be deleted.

OK, then lets keep the diff minimal invasive.  I'm ignoring the DPRINTF
part for now.


Index: sys/dev/sdmmc/sdmmc_scsi.c
===================================================================
RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_scsi.c,v
retrieving revision 1.59
diff -u -p -u -p -r1.59 sdmmc_scsi.c
--- sys/dev/sdmmc/sdmmc_scsi.c  15 Oct 2020 13:22:13 -0000      1.59
+++ sys/dev/sdmmc/sdmmc_scsi.c  23 Mar 2021 14:45:53 -0000
@@ -366,7 +366,7 @@ sdmmc_scsi_cmd(struct scsi_xfer *xs)
        sdmmc_scsi_decode_rw(xs, &blockno, &blockcnt);
 
        if (blockno >= tgt->card->csd.capacity ||
-           blockno + blockcnt > tgt->card->csd.capacity) {
+           blockno + blockcnt >= tgt->card->csd.capacity) {
                DPRINTF(("%s: out of bounds %u-%u >= %u\n", DEVNAME(sc),
                    blockno, blockcnt, tgt->card->csd.capacity));
                xs->error = XS_DRIVER_STUFFUP;