On 07/12/2015 12:14 AM, Dan Carpenter wrote:
> Hello Hannes Reinecke,
>
> The patch b6ff1b14cdf4: "[SCSI] scsi_dh: Update EMC handler" from Jul
> 17, 2008, leads to the following static checker warning:
>
> drivers/scsi/device_handler/scsi_dh_emc.c:252 parse_sp_model()
> warn: buffer overflow 'buffer' 252 <= 255
>
> drivers/scsi/device_handler/scsi_dh_emc.c
> 217 static char * parse_sp_model(struct scsi_device *sdev, unsigned char
> *buffer)
> 218 {
> 219 unsigned char len = buffer[4] + 5;
>
> The warning is simply because Smatch assumes that "len" can be up to 255
> since it is a u8. That is likely not a real concern but I think there
> are some off by one errors.
>
> 220 char *sp_model = NULL;
> 221 unsigned char sp_len, serial_len;
> 222
> 223 if (len < 160) {
>
> These conditions seem off by one to me. If len == 160 then we can read
> up to buffer[159]? I think this should be:
>
> if (len < 161).
>
> 224 sdev_printk(KERN_WARNING, sdev,
> 225 "%s: Invalid information section length
> %d\n",
> 226 CLARIION_NAME, len);
> 227 /* Check for old FC arrays */
> 228 if (!strncmp(buffer + 8, "DGC", 3)) {
> 229 /* Old FC array, not supporting extended
> information */
> 230 sp_model = emc_default_str;
> 231 }
> 232 goto out;
> 233 }
> 234
> 235 /*
> 236 * Parse extended information for SP model number
> 237 */
> 238 serial_len = buffer[160];
> 239 if (serial_len == 0 || serial_len + 161 > len) {
>
> Here the > should probably be >=.
>
> 240 sdev_printk(KERN_WARNING, sdev,
> 241 "%s: Invalid array serial number length
> %d\n",
> 242 CLARIION_NAME, serial_len);
> 243 goto out;
> 244 }
> 245 sp_len = buffer[99];
> 246 if (sp_len == 0 || serial_len + sp_len + 161 > len) {
>
> And here as well.
>
> 247 sdev_printk(KERN_WARNING, sdev,
> 248 "%s: Invalid model number length %d\n",
> 249 CLARIION_NAME, sp_len);
> 250 goto out;
> 251 }
> 252 sp_model = &buffer[serial_len + 161];
>
>
> Otherwise we are potetially reading from &buffer[len] here which looks
> off by one.
>
> 253 /* Strip whitespace at the end */
> 254 while (sp_len > 1 && sp_model[sp_len - 1] == ' ')
> 255 sp_len--;
> 256
> 257 sp_model[sp_len] = '\0';
> 258
> 259 out:
> 260 return sp_model;
> 261 }
>
Thanks, I'll be sending a patch for it.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
