Bug 241929

Summary: ses(4) fails to sanitize illegal strings in SES element descriptors
Product: Base System Reporter: Alan Somers <asomers>
Component: binAssignee: Alan Somers <asomers>
Status: Closed FIXED    
Severity: Affects Many People CC: allanjude, bapt, emaste, juraj, trasz
Priority: ---    
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
sanitize SES element descriptors in ses(4)
none
sanitize SES element descriptors in ses(4) none

Description Alan Somers freebsd_committer 2019-11-12 21:34:56 UTC
"sesutil --libxo" is awesome.  It's very easy to parse, but ONLY if the enclosure's element descriptors are all valid UTF-8.  Unfortunately, we can't rely on that.  If an element descriptor is not valid UTF-8, then the JSON or XML output is not parseable.  Here's a real-world example:

$ sesutil map --libxo xml,pretty -u /dev/ses0
<sesutil version="1">
  <enclosures>
    <enc>ses0</enc>
    <name>SMC946 C1 0a01</name>
    ...
    <id>500304800924823f</id>
    <elements>
      <id>33</id>
      <type>Enclosure</type>
      <status>OK</status>
      <status_code>0x01 0x00 0x00 0x00</status_code>
      <description>��������������������������������</description>
    </elements>
    ...
  </enclosures>
</sesutil>

Those mystery characters are all 0xFF bytes.  sesutil needs to sanitize those fields before it prints them.
Comment 1 Allan Jude freebsd_committer 2019-11-12 23:13:33 UTC
Thanks for the report, I'll take a look
Comment 2 Alan Somers freebsd_committer 2019-11-12 23:32:24 UTC
No need, allanjude; I'm on it.  I'm thinking that this is actually a kernel bug instead of a sesutil bug.  ses(4) should probably sanitize those fields as soon as it reads them.  Otherwise other programs besides just sesutil could be affected.
Comment 3 Alan Somers freebsd_committer 2019-11-13 20:26:03 UTC
It's definitely a kernel problem.  I have a supermicro expander that puts illegal characters in its SES element descriptors (SES diagnostic page 7).  The kernel faithfully forwards that garbage to consumers of the ENCIOC_GETELMDESC ioctl.  While sesutil(8) could theoretically sanitize those descriptors itself, I think it's better for the kernel to do it.  That's what the attached (not fully tested yet) patch does.
Comment 4 Alan Somers freebsd_committer 2019-11-13 20:26:33 UTC
Created attachment 209138 [details]
sanitize SES element descriptors in ses(4)
Comment 5 Juraj Lutter 2019-11-13 20:42:06 UTC
Just for the record,

elmpriv->descr_len = length

should be adjusted accordingly in csase "<invalid>" is returned.
Comment 6 Alan Somers freebsd_committer 2019-11-13 20:50:57 UTC
Created attachment 209139 [details]
sanitize SES element descriptors in ses(4)

Update elmpriv->descr in the case of an invalid descriptor.
Comment 7 Allan Jude freebsd_committer 2019-11-13 22:23:03 UTC
(In reply to Alan Somers from comment #6)
Thanks for the better fix.

Reviewed By: allanjude
Comment 8 commit-hook freebsd_committer 2019-12-06 00:06:10 UTC
A commit references this bug:

Author: asomers
Date: Fri Dec  6 00:06:06 UTC 2019
New revision: 355430
URL: https://svnweb.freebsd.org/changeset/base/355430

Log:
  ses: sanitize illegal strings in SES element descriptors

  The SES4r3 standard requires that element descriptors may only contain ASCII
  characters in the range 0x20 to 0x7e.  Some SuperMicro expanders violate
  that rule.  This patch adds a sanity check to ses(4).  Descriptors in
  violation will be replaced by "<invalid>".

  This patch fixes "sesutil --libxo xml" on such systems.  Previously it would
  generate non-well-formed XML output.

  PR:		241929
  Reviewed by:	allanjude
  MFC after:	2 weeks
  Sponsored by:	Axcient

Changes:
  head/sys/cam/scsi/scsi_enc_ses.c
Comment 9 commit-hook freebsd_committer 2020-02-13 20:40:48 UTC
A commit references this bug:

Author: asomers
Date: Thu Feb 13 20:40:37 UTC 2020
New revision: 357877
URL: https://svnweb.freebsd.org/changeset/base/357877

Log:
  MFC r355430:

  ses: sanitize illegal strings in SES element descriptors

  The SES4r3 standard requires that element descriptors may only contain ASCII
  characters in the range 0x20 to 0x7e.  Some SuperMicro expanders violate
  that rule.  This patch adds a sanity check to ses(4).  Descriptors in
  violation will be replaced by "<invalid>".

  This patch fixes "sesutil --libxo xml" on such systems.  Previously it would
  generate non-well-formed XML output.

  PR:		241929
  Reviewed by:	allanjude
  Sponsored by:	Axcient

Changes:
_U  stable/12/
  stable/12/sys/cam/scsi/scsi_enc_ses.c
Comment 10 commit-hook freebsd_committer 2020-03-22 02:54:26 UTC
A commit references this bug:

Author: asomers
Date: Sun Mar 22 02:53:44 UTC 2020
New revision: 359205
URL: https://svnweb.freebsd.org/changeset/base/359205

Log:
  MFC r355430:

  ses: sanitize illegal strings in SES element descriptors

  The SES4r3 standard requires that element descriptors may only contain ASCII
  characters in the range 0x20 to 0x7e.  Some SuperMicro expanders violate
  that rule.  This patch adds a sanity check to ses(4).  Descriptors in
  violation will be replaced by "<invalid>".

  This patch fixes "sesutil --libxo xml" on such systems.  Previously it would
  generate non-well-formed XML output.

  PR:		241929
  Reviewed by:	allanjude
  Sponsored by:	Axcient

Changes:
_U  stable/11/
  stable/11/sys/cam/scsi/scsi_enc_ses.c