Date: Fri, 2 Aug 1996 17:27:54 +1000 From: Bruce Evans <bde@zeta.org.au> To: CVS-committers@freefall.freebsd.org, cvs-all@freefall.freebsd.org, cvs-sys@freefall.freebsd.org, peter@freefall.freebsd.org Subject: Re: cvs commit: src/sys/scsi od.c sd.c Message-ID: <199608020727.RAA13989@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
> Modified: sys/scsi od.c sd.c
> Log:
> The sd and od drivers didn't check for negative block numbers (like wd.c
> does) before calling dscheck(). dscheck() doesn't appreciate this and
wd.c is incorrect. It's stupid to repeat this check in all drivers. A
complete check involves checking much more than the block number (see
below). Note that all the cd drivers still haven't been converted to
use dscheck(), so they call bounds_check_with_label(), and neither the
drivers nor bounds_check_with_label() check for negative block numbers.
> calls Debugger() and returns without setting bp->b_error.
It should set bp->b_error = EINVAL. It does set bp->b_flags |= B_ERROR.
This should be translated to bp->b_error = EIO if bp->b_error is 0.
Perhaps the critical bug is in biodone() or physio() not doing this
translation. physio() does the following:
if (bp->b_flags & B_ERROR) {
error = bp->b_error;
goto doerror;
}
...
doerror:
...
return (error);
The translation seems to only be done in biowait(). I now think that
there should be no translation. bp->b_flags & B_ERROR should never be
set without setting bp->b_error to nonzero.
> This can happen when there is a casting error and offsets > 2G are
> converted to negative off_t's in the disk tools. (dumpfs used to do this).
The bug for this is in physio(). physio() should check for unrepresentable
block numbers (which can result from both negative off_t's and off_t's
larger than 2**40) and the dscheck() should only check inside an
`#ifdef DIAGNOSTIC' section (unless the check can be folded into normal
checks).
dscheck() also has various overflow possibilities for block numbers
slightly less than the maximum. Overflows to a negative number may
occur for adding the partition offset, adding the slice offset and
adding the size. These were supposed to be checked for (at least on 2's
complement machines) provided overflows don't trap and disk sizes are
not near the maximum (maxsz is signless so disk regions than begin near
the maximum appear to be entirely beyond the disk). I can't see any
fatal errors in the check. If bp->b_blkno < 0, then the `sz <= 0' case
is reached and dscheck() sets bp->b_error = EINVAL and bp->b_flags |=
B_ERROR and returns an error.
bounds_check_with_label() has the same overflow possibilities but
they aren't checked for at all (maxsz is signed ...).
Bogus bp->b_count's also have interesting overflow possibilities.
E.g., bp->b_bcount = -1 is rounded "up" to sz = 0.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199608020727.RAA13989>