Mark Kettenis <mark.kette...@xs4all.nl> 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?
Hmm. Maybe this is a specific SD card with a bug accessing the last sector.
A manual dd using
skip='c partition size -1' count=1 bs=512
Is failing on that SD card, but it is working on other SD cards. Which appear
to all be SD v2 cards.
Maybe his specific SD card has an off-by-one bug relating to the last sector,
and we need to determine a way to handle that, or skip the last sector on
all devices of this sub-class?
I don't see any issue in the controller code. Marcus, can you move the card
to another machine (different controller) and use dd to read the last sector?
Though I suspect that might not provide enough clue either, the "fail to read"
behaviour might vary between controllers...
> > 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)
All 3 variables are int. Not sure how moving the + to - on the other
side changes anything.
