> From: "Theo de Raadt" <dera...@openbsd.org>
> Date: Tue, 23 Mar 2021 08:29:06 -0600
>
> 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);
Apart from a potential overflow, the original condition looks correct
to me. If csd.capacity is 16 and blockno is 0 and blockcnt is 16, the
operation should succeed, shouldn't it?
> blockno and blockcnt are both 32-bit, so this feels dangerously close
> to integer wrap, and I believe the first condition cannot be deleted.
But the second condition can still wrap. So it probably needs to be
something like:
if (blockno >= tgt->card->csd.capacity ||
blockcnt > tgt->card->csd.capacity - blockno)
Cheers,
Mark
