Hi Bart -
Thanks for taking the time to look this over.
On 02/27/2017 10:13 AM, Bart Van Assche wrote:
On Mon, 2017-02-27 at 09:22 -0600, Steven J. Magnani wrote:
@@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
return -ENODEV;
}
- if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
+ /* Make sure logical_to_sectors() won't overflow */
+ lba_in_sectors = lba << (ilog2(sector_size) - 9);
+ if ((sizeof(sdkp->capacity) == 4) &&
+ ((lba >= 0xffffffffULL) || (lba_in_sectors >= 0xffffffffULL))) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2162,6 +2165,7 @@ static int read_capacity_10(struct scsi_
int the_result;
int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
sector_t lba;
+ unsigned long long lba_in_sectors;
unsigned sector_size;
do {
@@ -2208,7 +2212,10 @@ static int read_capacity_10(struct scsi_
return sector_size;
}
- if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {
+ /* Make sure logical_to_sectors() won't overflow */
+ lba_in_sectors = ((unsigned long long) lba) << (ilog2(sector_size) - 9);
+ if ((sizeof(sdkp->capacity) == 4) &&
+ (lba_in_sectors >= 0xffffffffULL)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
Why are the two checks slightly different? Could the same code be used for
both checks?
The checks are different because with READ CAPACITY(16) a _really_ huge
device could report a max LBA so large that left-shifting it causes bits
to drop off the end. That's not an issue with READ CAPACITY(10) because
at most the 32-bit LBA reported by the device will become a 35-bit value
(since the max supported block size is 4096 == (512 << 3)).
BTW, using the macro below would make the above checks less
verbose and easier to read:
/*
* Test whether the result of a shift-left operation would be larger than
* what fits in a variable with the type of @a.
*/
#define shift_left_overflows(a, b) \
({ \
typeof(a) _minus_one = -1LL; \
typeof(a) _plus_one = 1; \
bool _a_is_signed = _minus_one < 0; \
int _shift = sizeof(a) * 8 - ((b) + _a_is_signed); \
_shift < 0 || ((a) & ~((_plus_one << _shift) - 1)) != 0;\
})
Bart.
Perhaps but I am not a fan of putting braces in non-obvious places such
as within array subscripts (which I've encountered recently) or
conditional expressions, which is what this amounts to.
Regards,
------------------------------------------------------------------------
Steven J. Magnani "I claim this network for MARS!
www.digidescorp.com Earthling, return my space modulator!"
#include <standard.disclaimer>
