NAS-142826 / 27.0.0-BETA.1 / drivetemp: fix on-stack DMA and positive SCSI status leak in LOG SENSE path - #348
Open
eschultz wants to merge 2 commits into
Conversation
drivetemp_retrieve_temp_log() declares the LOG SENSE data buffer on the stack: char buf[TEMP_LOG_PAGE_LEN]; and hands it straight to scsi_execute_cmd() as the data-in buffer (drivers/hwmon/drivetemp.c:340 and :348-349). Stack memory is not a legal DMA target. Documentation/core-api/dma-api-howto.rst:132-134 states: "This rule also means that you may use neither kernel image addresses (items in data/text/bss segments), nor module image addresses, nor stack addresses for DMA." With CONFIG_VMAP_STACK (default on x86_64, and set in the TrueNAS production config) the kernel stack is a vmalloc() mapping, which dma-api-howto.rst:125-126 calls out explicitly as unusable for DMA. Today this does not corrupt memory only because the block layer still carries a safety net: blk_rq_map_kern() routes the request through the copy path whenever object_is_on_stack(kbuf) is true (block/blk-map.c:774, object_is_on_stack() at include/linux/sched/task_stack.h:89). So every temperature poll silently allocates and copies a bounce buffer, and the driver's correctness rests on a bounce path the block layer is not obliged to keep. The rest of this driver does not rely on it: every ATA/SATA command DMAs into st->smartdata (drivers/hwmon/drivetemp.c:204- 205), a u8 smartdata[ATA_SECT_SIZE] member of the kzalloc()'d struct drivetemp_data (drivers/hwmon/drivetemp.c:118, allocated at :647). That is page-allocator-backed memory and a legal DMA target. Make the SCSI path do the same. ATA_SECT_SIZE is 512 (include/linux/ata.h:28) and TEMP_LOG_PAGE_LEN is 0x10 (drivers/hwmon/drivetemp.c:148), so the log page fits with room to spare. Sharing st->smartdata is safe. The only runtime caller, drivetemp_get_scsitemp(), is reached through st->get_temp() under st->lock (drivers/hwmon/drivetemp.c:549-551), the mutex documented at :114 as protecting data buffer accesses. The only other caller, drivetemp_identify_scsi(), runs from drivetemp_add() at :655, before hwmon_device_register_with_info() at :660 publishes the device, so no sysfs reader can race it. Two consequences of the buffer now being persistent are handled here: - Zero the bytes we are about to parse before issuing the command. scsi_execute_cmd() already zeroes the residual tail on a short transfer (drivers/scsi/scsi_lib.c:341-342), but that depends on the LLDD reporting resid_len correctly; without the memset a driver that does not would leave the parser reading last poll's temperature, or leftover SATA SMART data, instead of obviously-invalid zeros. - Bound the temperature read. The parameter walk only checks i + param_len <= page_len, but reads the temperature at buf[i + TEMP_LOG_PARAM_TEMP_OFFSET] == buf[i + 5], which a device reporting a parameter shorter than six bytes does not cover. On the stack that was a small over-read of a 16-byte array, catchable by KASAN; with a shared 512-byte buffer it would instead return plausible-looking stale bytes. Skip parameters too short to contain the temperature byte. The buffer type changes from char to u8. The parser already cast every byte it consumed to u8 (:357, :362, :364) and get_unaligned_be16() takes a const void *, so no parsing behaviour changes; the two now-redundant casts on the temperature reads are dropped. The CDB array stays on the stack: scsi_execute_cmd() memcpy()s it into scmd->cmnd (drivers/scsi/scsi_lib.c:318-319) and never DMAs from it, which is why drivetemp_scsi_command() does the same at drivers/hwmon/drivetemp.c:174. No functional change intended for well-behaved drives. Fixes: 06ff843 ("hwmon: (drivetemp) Add SCSI drives support") Signed-off-by: Eric Schultz <eric@startuperic.com>
drivetemp_retrieve_temp_log() returns the raw return value of
scsi_execute_cmd() to its callers:
err = scsi_execute_cmd(st->sdev, scsi_cmd, REQ_OP_DRV_IN, buf,
TEMP_LOG_PAGE_LEN, 10 * HZ, 5, NULL);
if (err)
return (err);
scsi_execute_cmd() is documented at drivers/scsi/scsi_lib.c:287-288 as
returning "the scsi_cmnd result field if a command was executed, or a
negative Linux error code if we didn't get that far", and the
implementation does exactly that at drivers/scsi/scsi_lib.c:352
("ret = scmd->result;"). A drive that terminates the LOG SENSE with
CHECK CONDITION therefore produces a *positive* return value, not an
errno.
That positive value propagates unmodified through
drivetemp_get_scsitemp() (drivers/hwmon/drivetemp.c:377-383, which only
tests "== 0") into drivetemp_read() and out to the hwmon core.
hwmon_attr_show() only rejects negative returns:
drivers/hwmon/hwmon.c:427 long val;
drivers/hwmon/hwmon.c:430-433 ret = hattr->ops->read(...., &val);
if (ret < 0)
return ret;
so a positive result is treated as success and the uninitialised stack
variable 'val' is formatted and handed to userspace as the drive
temperature. A SAS drive that fails LOG SENSE (media error, unit
attention after a reset, an offline device, or a drive that does not
implement log page 0x0d) thus reports a garbage temp1_input instead of
failing the read.
The SATA sibling in this same file already normalises correctly, at
drivers/hwmon/drivetemp.c:204-208:
err = scsi_execute_cmd(st->sdev, scsi_cmd, op, st->smartdata,
ATA_SECT_SIZE, 10 * HZ, 5, NULL);
if (err > 0)
err = -EIO;
return err;
Apply the same normalisation on the SCSI path. Like the SATA path, this
deliberately treats *any* CHECK CONDITION as a failed read: a LOG SENSE
completing with sense key NO SENSE or RECOVERED ERROR transferred usable
data but will now return -EIO rather than a temperature. That is a
conscious trade for not returning uninitialised stack data, and it
matches the existing behaviour of every other command this driver
issues.
The secondary caller, drivetemp_identify_scsi()
(drivers/hwmon/drivetemp.c:387-400), was already benign because
drivetemp_add() collapses any non-zero identify result to -ENODEV
(drivers/hwmon/drivetemp.c:655-658), but it now also gets a sane errno.
No functional change for drives whose LOG SENSE succeeds.
Fixes: 06ff843 ("hwmon: (drivetemp) Add SCSI drives support")
Signed-off-by: Eric Schultz <eric@startuperic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: https://ixsystems.atlassian.net/browse/NAS-142826
Split out of #347 at review request. This PR contains only the two bug fixes from that series — no ABI change, no behaviour change on a drive whose LOG SENSE succeeds. The
temp1_crit→temp1_maxretier and the subpage 02h read stay on #347.Both bugs are in
drivetemp_retrieve_temp_log(), the SCSI log-page path added in 06ff843.1. do not DMA into an on-stack buffer for LOG SENSE
The 16-byte log buffer is declared on the stack and handed to
scsi_execute_cmd()as the data-in buffer. Stack memory is not a legal DMA target (Documentation/core-api/dma-api-howto.rst), and withCONFIG_VMAP_STACK=y— set in the production config — the stack is a vmalloc mapping, which that document calls out explicitly. It works today only becauseblk_rq_map_kern()bounces the request whenobject_is_on_stack()is true, so every temperature poll silently allocates and copies, and correctness rests on a block-layer safety net. Every ATA command in this same driver already DMAs intost->smartdata; this makes the SCSI path do the same.Two consequences of the buffer becoming persistent are handled: the parsed bytes are zeroed before the command is issued (so a LLDD that misreports
resid_lenyields obvious zeros rather than last poll's temperature), and the temperature read is bounded (the parameter walk checksi + param_len <= page_lenbut reads atbuf[i + 5], which a parameter shorter than six bytes does not cover — harmless on a 16-byte stack array, stale plausible data in a shared 512-byte buffer).2. do not leak positive SCSI status as an errno
scsi_execute_cmd()returns the positivescmd->resulton CHECK CONDITION, not an errno.hwmon_attr_show()only rejects negative returns, so that positive value is treated as success and the uninitialised stacklong valis formatted and handed to userspace as the drive temperature.This is the same defect upstream fixed on the ATA path in 82163d6 ("hwmon: (drivetemp) Fix driver producing garbage data when SCSI errors occur", Daniil Stas, v6.13) — that commit is the
if (err > 0) err = -EIO;atdrivetemp.c:206on this branch, and we also carry it ontruenas/linux-6.12as 42268d8. The SCSI log-page path was added after it and never got the same normalisation. This restores it.Like the ATA path, this treats any CHECK CONDITION as a failed read, including NO SENSE / RECOVERED ERROR. That is the existing behaviour of every other command this driver issues.
Base
truenas/linux-6.18@ 580caa3. Applies totruenas/linux-6.12unchanged withFixes: 43ff910ef5cc.Verification:
drivers/hwmon/drivetemp.ocross-compiles clean for x86_64, includingW=1, onx86_64_defconfig+CONFIG_TRUENAS=y+CONFIG_SENSORS_DRIVETEMP=m(CONFIG_VMAP_STACK=yconfirmed set).checkpatch.pl --strictreports no issues on either patch. Not run on hardware; neither change alters the success path, so a healthy drive reports the same temperature it does today.