Coverity detected issues in SeaBIOS

[Ian Campbell]

Hello,

We run Coverity on the Xen source code occasionally and it happens to
include SeaBIOS. The following new warnings have appeared since I pulled
in rel-1.7.5.

At least the MISSING_BREAK ones look likely to be valid to me. Not sure
about the other two...

Ian.

-------- Forwarded Message --------
From: [e-mail]
Subject: New Defects reported by Coverity Scan for XenProject
Date: Wed, 16 Jul 2014 07:16:19 -0700
Message-id:

Hi,

Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.

Defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)

** CID 1226281:  Unchecked return value  (CHECKED_RETURN)
/tools/firmware/seabios-dir-remote/src/fw/smbios.c: 578 in smbios_legacy_setup()

** CID 1226282:  Missing break in switch  (MISSING_BREAK)
/tools/firmware/seabios-dir-remote/src/hw/blockcmd.c: 49 in cdb_cmd_data()

** CID 1226283:  Missing break in switch  (MISSING_BREAK)
/tools/firmware/seabios-dir-remote/src/hw/blockcmd.c: 52 in cdb_cmd_data()

** CID 1226284:  Missing break in switch  (MISSING_BREAK)
/tools/firmware/seabios-dir-remote/src/hw/blockcmd.c: 55 in cdb_cmd_data()

** CID 1226285:  Unintentional integer overflow  (OVERFLOW_BEFORE_WIDEN)
/tools/firmware/seabios-dir-remote/src/hw/usb-xhci.c: 698 in configure_xhci()

________________________________________________________________________________________________________
*** CID 1226281:  Unchecked return value  (CHECKED_RETURN)
/tools/firmware/seabios-dir-remote/src/fw/smbios.c: 578 in smbios_legacy_setup()
572             }
573         }
574     
575         add_struct(32, p);
576         /* Add any remaining provided entries before the end marker */
577         for (i = 0; i < 256; i++)
> > > CID 1226281:  Unchecked return value  (CHECKED_RETURN)
> > > Calling "get_external" without checking return value (as is done elsewhere 12 out of 13 times).

578             get_external(i, &p, &nr_structs, &max_struct_size, end);
579         add_struct(127, p);
580     
581     #undef add_struct
582     
583         smbios_entry_point_setup(max_struct_size, p - start, start, nr_structs);
584         free(start);

________________________________________________________________________________________________________
*** CID 1226282:  Missing break in switch  (MISSING_BREAK)
/tools/firmware/seabios-dir-remote/src/hw/blockcmd.c: 49 in cdb_cmd_data()
43             return esp_scsi_cmd_data(op, cdbcmd, blocksize);
44         case DTYPE_MEGASAS:
45             return megasas_cmd_data(op, cdbcmd, blocksize);
46         case DTYPE_USB_32:
47             if (!MODESEGMENT)
48                 return usb_cmd_data(op, cdbcmd, blocksize);
> > > CID 1226282:  Missing break in switch  (MISSING_BREAK)
> > > The above case falls through to this one.

49         case DTYPE_UAS_32:
50             if (!MODESEGMENT)
51                 return uas_cmd_data(op, cdbcmd, blocksize);
52         case DTYPE_PVSCSI:
53             if (!MODESEGMENT)
54                 return pvscsi_cmd_data(op, cdbcmd, blocksize);

________________________________________________________________________________________________________
*** CID 1226283:  Missing break in switch  (MISSING_BREAK)
/tools/firmware/seabios-dir-remote/src/hw/blockcmd.c: 52 in cdb_cmd_data()
46         case DTYPE_USB_32:
47             if (!MODESEGMENT)
48                 return usb_cmd_data(op, cdbcmd, blocksize);
49         case DTYPE_UAS_32:
50             if (!MODESEGMENT)
51                 return uas_cmd_data(op, cdbcmd, blocksize);
> > > CID 1226283:  Missing break in switch  (MISSING_BREAK)
> > > The above case falls through to this one.

52         case DTYPE_PVSCSI:
53             if (!MODESEGMENT)
54                 return pvscsi_cmd_data(op, cdbcmd, blocksize);
55         case DTYPE_AHCI_ATAPI:
56             if (!MODESEGMENT)
57                 return ahci_cmd_data(op, cdbcmd, blocksize);

________________________________________________________________________________________________________
*** CID 1226284:  Missing break in switch  (MISSING_BREAK)
/tools/firmware/seabios-dir-remote/src/hw/blockcmd.c: 55 in cdb_cmd_data()
49         case DTYPE_UAS_32:
50             if (!MODESEGMENT)
51                 return uas_cmd_data(op, cdbcmd, blocksize);
52         case DTYPE_PVSCSI:
53             if (!MODESEGMENT)
54                 return pvscsi_cmd_data(op, cdbcmd, blocksize);
> > > CID 1226284:  Missing break in switch  (MISSING_BREAK)
> > > The above case falls through to this one.

55         case DTYPE_AHCI_ATAPI:
56             if (!MODESEGMENT)
57                 return ahci_cmd_data(op, cdbcmd, blocksize);
58         default:
59             return DISK_RET_EPARAM;
60         }

________________________________________________________________________________________________________
*** CID 1226285:  Unintentional integer overflow  (OVERFLOW_BEFORE_WIDEN)
/tools/firmware/seabios-dir-remote/src/hw/usb-xhci.c: 698 in configure_xhci()
692                 free(spba);
693                 free(pad);
694                 goto fail;
695             }
696             int i;
697             for (i = 0; i < spb; i++)
> > > CID 1226285:  Unintentional integer overflow  (OVERFLOW_BEFORE_WIDEN)
> > > Potentially overflowing expression "i * 4096" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic before being used in a context which expects an expression of type "u64" (64 bits, unsigned). To avoid overflow, cast either operand to "u64" before performing the multiplication.

698                 spba[i] = (u32)pad + (i * PAGE_SIZE);
699             xhci->devs[0].ptr_low = (u32)spba;
700             xhci->devs[0].ptr_high = 0;
701         }
702     
703         reg = readl(&xhci->op->usbcmd);

________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, http://scan.coverity.com/projects/606?tab=overview

To unsubscribe from the email notification for new defects, http://scan5.coverity.com/cgi-bin/unsubscribe.py

[Kevin O'Connor]

On Wed, Jul 16, 2014 at 04:27:18PM +0100, Ian Campbell wrote:
> Hello,
>
> We run Coverity on the Xen source code occasionally and it happens to
> include SeaBIOS. The following new warnings have appeared since I pulled
> in rel-1.7.5.

Thanks.  All five look like false positives to me.  I'm happy to take
patches if you want to rework the code to prevent the warnings.
> At least the MISSING_BREAK ones look likely to be valid to me. Not sure
> about the other two...

It's a bit ugly, but it should be okay because all three cases start
with "if (!MODESEGMENT)" which is a compile time constant.  So, when
compiled in 32bit mode the three cases will each return the results
from their respective functions, and in 16bit mode all three will
return DISK_RET_EPARAM.

-Kevin

[Ian Campbell]

On Wed, 2014-07-16 at 13:33 -0400, Kevin O'Connor wrote:
> On Wed, Jul 16, 2014 at 04:27:18PM +0100, Ian Campbell wrote:
> > Hello,
> >
> > We run Coverity on the Xen source code occasionally and it happens to
> > include SeaBIOS. The following new warnings have appeared since I pulled
> > in rel-1.7.5.
>
> Thanks.  All five look like false positives to me.

OK, we will tag them as such for our runs. Thanks.
> I'm happy to take
> patches if you want to rework the code to prevent the warnings.
> > At least the MISSING_BREAK ones look likely to be valid to me. Not sure
> > about the other two...
>
> It's a bit ugly, but it should be okay because all three cases start
> with "if (!MODESEGMENT)" which is a compile time constant.  So, when
> compiled in 32bit mode the three cases will each return the results
> from their respective functions, and in 16bit mode all three will
> return DISK_RET_EPARAM.

Subtle!

[Markus Armbruster]

"Kevin O'Connor"  writes:
> On Wed, Jul 16, 2014 at 04:27:18PM +0100, Ian Campbell wrote:
> > Hello,
> >
> > We run Coverity on the Xen source code occasionally and it happens to
> > include SeaBIOS. The following new warnings have appeared since I pulled
> > in rel-1.7.5.
>
> Thanks.  All five look like false positives to me.  I'm happy to take
> patches if you want to rework the code to prevent the warnings.
> > At least the MISSING_BREAK ones look likely to be valid to me. Not sure
> > about the other two...
>
> It's a bit ugly, but it should be okay because all three cases start
> with "if (!MODESEGMENT)" which is a compile time constant.  So, when
> compiled in 32bit mode the three cases will each return the results
> from their respective functions, and in 16bit mode all three will
> return DISK_RET_EPARAM.

A comment would make your intention clearn and shut up Coverity:

case DTYPE_USB_32:
            if (!MODESEGMENT)
                return usb_cmd_data(op, cdbcmd, blocksize);
            /* fall through */
        case DTYPE_UAS_32:
            if (!MODESEGMENT)
                return uas_cmd_data(op, cdbcmd, blocksize);