Bug 215474 - ccb_pathinq.(sim_vid|hba_vid|dev_name) aren't null terminated
Summary: ccb_pathinq.(sim_vid|hba_vid|dev_name) aren't null terminated
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-21 18:07 UTC by Alan Somers
Modified: 2017-04-24 16:20 UTC (History)
1 user (show)

See Also:
asomers: mfc-stable11+
asomers: mfc-stable10-

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Somers freebsd_committer freebsd_triage 2016-12-21 18:07:50 UTC 
The sim_vid, hba_vid, and dev_name fields of struct ccb_pathinq are fixed-length strings.  AFAICT the only place they're read is in sbin/camcontrol/camcontrol.c, which assumes they'll be null-terminated.  However, the kernel doesn't null-terminate them. A bunch of copy-pasted code uses strncpy to write them, and doesn't guarantee null-termination.  For at least 4 drivers (mpr, mps, ciss, and hyperv), the hba_vid field actually overflows.  You can see the result by doing "camcontrol negotiate da0 -v".  Note the garbage at the end of the HBA vendor field:

$ camcontrol negotiate da0 -v
Current parameters:
(pass0:mps0:0:10:0): transfer speed: 300.000MB/s
(pass0:mps0:0:10:0): tagged queueing: enabled
mps0: SIM/HBA version: 1
mps0: supports tag queue messages
mps0: supports SDTR message
mps0: supports 16 bit wide SCSI
mps0: unknown PIM bit set
mps0: unknown PIM bit set
mps0: user has disabled initial BUS RESET or controller is in target/mixed mode
mps0: HBA engine count: 0
mps0: maximum target: 255
mps0: maximum LUN: 255
mps0: highest path ID in subsystem: 0
mps0: initiator ID: 255
mps0: SIM vendor: FreeBSD
mps0: HBA vendor: Avago Tech (LSI)mps
mps0: HBA vendor ID: 0x0000
mps0: HBA device ID: 0x0000
mps0: HBA subvendor ID: 0x0000
mps0: HBA subdevice ID: 0x0000
mps0: bus ID: 0
mps0: base transfer speed: 150.000MB/sec
mps0: maximum transfer size: 4722688 bytes

The correct solution is to replace strncpy with strlcpy to null-terminate them, and shorten the hard-coded strings that are too long.
Comment 1 Alan Somers freebsd_committer freebsd_triage 2016-12-22 00:14:01 UTC 
The following Coverity CIDs relate to this issue:

1009997 1010000 1010001 1010002 1010003 1010004 1010005 1331519 1010006 1215097
1010007 1288967 1010008 1306000 1211924 1010009 1010010 1010011 1010012 1010013
1010014 1147190 1010017 1010016 1010018 1216435 1010020 1010021 1010022 1009666
1018185 1010023 1010025 1010026 1010027 1010028 1010029 1010030 1010031 1010033
1018186 1018187 1010035 1010036 1010042 1010041 1010040 1010039
Comment 2 commit-hook freebsd_committer freebsd_triage 2017-01-04 20:27:00 UTC 
A commit references this bug:

Author: asomers
Date: Wed Jan  4 20:26:48 UTC 2017
New revision: 311305
URL: https://svnweb.freebsd.org/changeset/base/311305

Log:
  Always null-terminate ccb_pathinq.(sim_vid|hba_vid|dev_name)

  The sim_vid, hba_vid, and dev_name fields of struct ccb_pathinq are
  fixed-length strings. AFAICT the only place they're read is in
  sbin/camcontrol/camcontrol.c, which assumes they'll be null-terminated.
  However, the kernel doesn't null-terminate them. A bunch of copy-pasted code
  uses strncpy to write them, and doesn't guarantee null-termination. For at
  least 4 drivers (mpr, mps, ciss, and hyperv), the hba_vid field actually
  overflows. You can see the result by doing "camcontrol negotiate da0 -v".

  This change null-terminates those fields everywhere they're set in the
  kernel. It also shortens a few strings to ensure they'll fit within the
  16-character field.

  PR:		215474
  Reported by:	Coverity
  CID:		1009997 1010000 1010001 1010002 1010003 1010004 1010005
  CID:		1331519 1010006 1215097 1010007 1288967 1010008 1306000
  CID:		1211924 1010009 1010010 1010011 1010012 1010013 1010014
  CID:		1147190 1010017 1010016 1010018 1216435 1010020 1010021
  CID:		1010022 1009666 1018185 1010023 1010025 1010026 1010027
  CID:		1010028 1010029 1010030 1010031 1010033 1018186 1018187
  CID:		1010035 1010036 1010042 1010041 1010040 1010039
  Reviewed by:	imp, sephe, slm
  MFC after:	4 weeks
  Sponsored by:	Spectra Logic Corp
  Differential Revision:	https://reviews.freebsd.org/D9037
  Differential Revision:	https://reviews.freebsd.org/D9038

Changes:
  head/sys/cam/cam_xpt.c
  head/sys/cam/ctl/ctl_frontend_cam_sim.c
  head/sys/cam/scsi/scsi_low.c
  head/sys/dev/aac/aac_cam.c
  head/sys/dev/aacraid/aacraid_cam.c
  head/sys/dev/advansys/advansys.c
  head/sys/dev/advansys/adwcam.c
  head/sys/dev/aha/aha.c
  head/sys/dev/ahb/ahb.c
  head/sys/dev/ahci/ahci.c
  head/sys/dev/ahci/ahciem.c
  head/sys/dev/aic/aic.c
  head/sys/dev/aic7xxx/aic79xx_osm.c
  head/sys/dev/aic7xxx/aic7xxx_osm.c
  head/sys/dev/amr/amr_cam.c
  head/sys/dev/arcmsr/arcmsr.c
  head/sys/dev/ata/ata-all.c
  head/sys/dev/buslogic/bt.c
  head/sys/dev/ciss/ciss.c
  head/sys/dev/dpt/dpt_scsi.c
  head/sys/dev/esp/ncr53c9x.c
  head/sys/dev/firewire/sbp.c
  head/sys/dev/firewire/sbp_targ.c
  head/sys/dev/hpt27xx/hpt27xx_osm_bsd.c
  head/sys/dev/hptiop/hptiop.c
  head/sys/dev/hptmv/entry.c
  head/sys/dev/hptnr/hptnr_osm_bsd.c
  head/sys/dev/hptrr/hptrr_osm_bsd.c
  head/sys/dev/hyperv/storvsc/hv_storvsc_drv_freebsd.c
  head/sys/dev/iir/iir.c
  head/sys/dev/isci/isci_controller.c
  head/sys/dev/iscsi_initiator/isc_cam.c
  head/sys/dev/isp/isp_freebsd.c
  head/sys/dev/mfi/mfi_cam.c
  head/sys/dev/mly/mly.c
  head/sys/dev/mpr/mpr_sas.c
  head/sys/dev/mps/mps_sas.c
  head/sys/dev/mpt/mpt_cam.c
  head/sys/dev/mrsas/mrsas_cam.c
  head/sys/dev/mvs/mvs.c
  head/sys/dev/ncr/ncr.c
  head/sys/dev/nvme/nvme_sim.c
  head/sys/dev/pms/freebsd/driver/ini/src/agtiapi.c
  head/sys/dev/ppbus/vpo.c
  head/sys/dev/siis/siis.c
  head/sys/dev/sym/sym_hipd.c
  head/sys/dev/trm/trm.c
  head/sys/dev/twa/tw_osl_cam.c
  head/sys/dev/tws/tws_cam.c
  head/sys/dev/virtio/scsi/virtio_scsi.c
  head/sys/powerpc/ps3/ps3cdrom.c
  head/sys/powerpc/pseries/phyp_vscsi.c
Comment 3 commit-hook freebsd_committer freebsd_triage 2017-03-23 06:41:11 UTC 
A commit references this bug:

Author: mav
Date: Thu Mar 23 06:40:26 UTC 2017
New revision: 315812
URL: https://svnweb.freebsd.org/changeset/base/315812

Log:
  MFC r311305 (by asomers):
  Always null-terminate ccb_pathinq.(sim_vid|hba_vid|dev_name)

  The sim_vid, hba_vid, and dev_name fields of struct ccb_pathinq are
  fixed-length strings. AFAICT the only place they're read is in
  sbin/camcontrol/camcontrol.c, which assumes they'll be null-terminated.
  However, the kernel doesn't null-terminate them. A bunch of copy-pasted code
  uses strncpy to write them, and doesn't guarantee null-termination. For at
  least 4 drivers (mpr, mps, ciss, and hyperv), the hba_vid field actually
  overflows. You can see the result by doing "camcontrol negotiate da0 -v".

  This change null-terminates those fields everywhere they're set in the
  kernel. It also shortens a few strings to ensure they'll fit within the
  16-character field.

  PR:		215474
  Reported by:    Coverity
  CID:            1009997 1010000 1010001 1010002 1010003 1010004 1010005
  CID:            1331519 1010006 1215097 1010007 1288967 1010008 1306000
  CID:            1211924 1010009 1010010 1010011 1010012 1010013 1010014
  CID:            1147190 1010017 1010016 1010018 1216435 1010020 1010021
  CID:            1010022 1009666 1018185 1010023 1010025 1010026 1010027
  CID:            1010028 1010029 1010030 1010031 1010033 1018186 1018187
  CID:            1010035 1010036 1010042 1010041 1010040 1010039

Changes:
_U  stable/11/
  stable/11/sys/cam/cam_xpt.c
  stable/11/sys/cam/ctl/ctl_frontend_cam_sim.c
  stable/11/sys/cam/scsi/scsi_low.c
  stable/11/sys/dev/aac/aac_cam.c
  stable/11/sys/dev/aacraid/aacraid_cam.c
  stable/11/sys/dev/advansys/advansys.c
  stable/11/sys/dev/advansys/adwcam.c
  stable/11/sys/dev/aha/aha.c
  stable/11/sys/dev/ahb/ahb.c
  stable/11/sys/dev/ahci/ahci.c
  stable/11/sys/dev/ahci/ahciem.c
  stable/11/sys/dev/aic/aic.c
  stable/11/sys/dev/aic7xxx/aic79xx_osm.c
  stable/11/sys/dev/aic7xxx/aic7xxx_osm.c
  stable/11/sys/dev/amr/amr_cam.c
  stable/11/sys/dev/arcmsr/arcmsr.c
  stable/11/sys/dev/ata/ata-all.c
  stable/11/sys/dev/buslogic/bt.c
  stable/11/sys/dev/ciss/ciss.c
  stable/11/sys/dev/dpt/dpt_scsi.c
  stable/11/sys/dev/esp/ncr53c9x.c
  stable/11/sys/dev/firewire/sbp.c
  stable/11/sys/dev/firewire/sbp_targ.c
  stable/11/sys/dev/hpt27xx/hpt27xx_osm_bsd.c
  stable/11/sys/dev/hptiop/hptiop.c
  stable/11/sys/dev/hptmv/entry.c
  stable/11/sys/dev/hptnr/hptnr_osm_bsd.c
  stable/11/sys/dev/hptrr/hptrr_osm_bsd.c
  stable/11/sys/dev/hyperv/storvsc/hv_storvsc_drv_freebsd.c
  stable/11/sys/dev/iir/iir.c
  stable/11/sys/dev/isci/isci_controller.c
  stable/11/sys/dev/iscsi_initiator/isc_cam.c
  stable/11/sys/dev/isp/isp_freebsd.c
  stable/11/sys/dev/mfi/mfi_cam.c
  stable/11/sys/dev/mly/mly.c
  stable/11/sys/dev/mpr/mpr_sas.c
  stable/11/sys/dev/mps/mps_sas.c
  stable/11/sys/dev/mpt/mpt_cam.c
  stable/11/sys/dev/mrsas/mrsas_cam.c
  stable/11/sys/dev/mvs/mvs.c
  stable/11/sys/dev/ncr/ncr.c
  stable/11/sys/dev/pms/freebsd/driver/ini/src/agtiapi.c
  stable/11/sys/dev/ppbus/vpo.c
  stable/11/sys/dev/siis/siis.c
  stable/11/sys/dev/sym/sym_hipd.c
  stable/11/sys/dev/trm/trm.c
  stable/11/sys/dev/twa/tw_osl_cam.c
  stable/11/sys/dev/tws/tws_cam.c
  stable/11/sys/dev/virtio/scsi/virtio_scsi.c
  stable/11/sys/powerpc/ps3/ps3cdrom.c
  stable/11/sys/powerpc/pseries/phyp_vscsi.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2017-03-23 06:42:13 UTC 
A commit references this bug:

Author: mav
Date: Thu Mar 23 06:41:19 UTC 2017
New revision: 315813
URL: https://svnweb.freebsd.org/changeset/base/315813

Log:
  MFC r311305 (by asomers):
  Always null-terminate ccb_pathinq.(sim_vid|hba_vid|dev_name)

  The sim_vid, hba_vid, and dev_name fields of struct ccb_pathinq are
  fixed-length strings. AFAICT the only place they're read is in
  sbin/camcontrol/camcontrol.c, which assumes they'll be null-terminated.
  However, the kernel doesn't null-terminate them. A bunch of copy-pasted code
  uses strncpy to write them, and doesn't guarantee null-termination. For at
  least 4 drivers (mpr, mps, ciss, and hyperv), the hba_vid field actually
  overflows. You can see the result by doing "camcontrol negotiate da0 -v".

  This change null-terminates those fields everywhere they're set in the
  kernel. It also shortens a few strings to ensure they'll fit within the
  16-character field.

  PR:             215474
  Reported by:    Coverity
  CID:            1009997 1010000 1010001 1010002 1010003 1010004 1010005
  CID:            1331519 1010006 1215097 1010007 1288967 1010008 1306000
  CID:            1211924 1010009 1010010 1010011 1010012 1010013 1010014
  CID:            1147190 1010017 1010016 1010018 1216435 1010020 1010021
  CID:            1010022 1009666 1018185 1010023 1010025 1010026 1010027
  CID:            1010028 1010029 1010030 1010031 1010033 1018186 1018187
  CID:            1010035 1010036 1010042 1010041 1010040 1010039

Changes:
_U  stable/10/
  stable/10/sys/cam/cam_xpt.c
  stable/10/sys/cam/ctl/ctl_frontend_cam_sim.c
  stable/10/sys/cam/scsi/scsi_low.c
  stable/10/sys/dev/aac/aac_cam.c
  stable/10/sys/dev/aacraid/aacraid_cam.c
  stable/10/sys/dev/advansys/advansys.c
  stable/10/sys/dev/advansys/adwcam.c
  stable/10/sys/dev/aha/aha.c
  stable/10/sys/dev/ahb/ahb.c
  stable/10/sys/dev/ahci/ahci.c
  stable/10/sys/dev/ahci/ahciem.c
  stable/10/sys/dev/aic/aic.c
  stable/10/sys/dev/aic7xxx/aic79xx_osm.c
  stable/10/sys/dev/aic7xxx/aic7xxx_osm.c
  stable/10/sys/dev/amr/amr_cam.c
  stable/10/sys/dev/arcmsr/arcmsr.c
  stable/10/sys/dev/ata/ata-all.c
  stable/10/sys/dev/buslogic/bt.c
  stable/10/sys/dev/ciss/ciss.c
  stable/10/sys/dev/dpt/dpt_scsi.c
  stable/10/sys/dev/esp/ncr53c9x.c
  stable/10/sys/dev/firewire/sbp.c
  stable/10/sys/dev/firewire/sbp_targ.c
  stable/10/sys/dev/hpt27xx/hpt27xx_osm_bsd.c
  stable/10/sys/dev/hptiop/hptiop.c
  stable/10/sys/dev/hptmv/entry.c
  stable/10/sys/dev/hptnr/hptnr_osm_bsd.c
  stable/10/sys/dev/hptrr/hptrr_osm_bsd.c
  stable/10/sys/dev/hyperv/storvsc/hv_storvsc_drv_freebsd.c
  stable/10/sys/dev/iir/iir.c
  stable/10/sys/dev/isci/isci_controller.c
  stable/10/sys/dev/iscsi_initiator/isc_cam.c
  stable/10/sys/dev/isp/isp_freebsd.c
  stable/10/sys/dev/mfi/mfi_cam.c
  stable/10/sys/dev/mly/mly.c
  stable/10/sys/dev/mpr/mpr_sas.c
  stable/10/sys/dev/mps/mps_sas.c
  stable/10/sys/dev/mpt/mpt_cam.c
  stable/10/sys/dev/mrsas/mrsas_cam.c
  stable/10/sys/dev/mvs/mvs.c
  stable/10/sys/dev/pms/freebsd/driver/ini/src/agtiapi.c
  stable/10/sys/dev/ppbus/vpo.c
  stable/10/sys/dev/siis/siis.c
  stable/10/sys/dev/sym/sym_hipd.c
  stable/10/sys/dev/trm/trm.c
  stable/10/sys/dev/twa/tw_osl_cam.c
  stable/10/sys/dev/tws/tws_cam.c
  stable/10/sys/dev/virtio/scsi/virtio_scsi.c
  stable/10/sys/powerpc/ps3/ps3cdrom.c
  stable/10/sys/powerpc/pseries/phyp_vscsi.c
Comment 5 Alan Somers freebsd_committer freebsd_triage 2017-04-24 16:20:55 UTC 
Fixed, and MFCed to stable/11