fix(sg,cam): do not dereference a NULL supported-device table - #651
Open
abdelhaleemahmed wants to merge 1 commit into
Open
abdelhaleemahmed wants to merge 1 commit into
abdelhaleemahmed wants to merge 1 commit into
Conversation
get_supported_devs() returns NULL for any vendor that is not IBM, HP,
HPE or QUANTUM - the switch has no default case and the initialiser is
NULL. Two backends use the result without checking it:
struct supported_device **cur = get_supported_devs(priv->vendor);
while(*cur) {
so opening a drive whose INQUIRY vendor id is not one of those four
segfaults instead of returning -EDEV_DEVICE_UNSUPPORTABLE three lines
further down.
The iokit backend already guards it with `while(cur && *cur)`; this
makes the sg and cam backends agree with it.
Reproduced on Linux with mhvtl, which lets a virtual drive present any
vendor id: a drive reporting vendor 'STK' with product 'ULT3580-TD8'
crashed `ltfs -o devname=... <mountpoint>` with SIGSEGV. With this
change the same command logs
LTFS30213I Unsupported Drive 'STK ' / 'ULT3580-TD8 '.
and exits 1.
The device list is unaffected: sg_get_device_list() names drives through
_generate_product_name(), which matches on product id alone, so such a
drive is listed and only refused when it is opened.
Signed-off-by: Ahmed Abdelhaleem Ahmed <ahmedhal@gmail.com>
XV02
requested changes
Oct 1, 2026
XV02
left a comment
Contributor
There was a problem hiding this comment.
This change is necessary, but the problem occurs in more places when calling get_supported_devs that are not addressed here. Also it may be a better fix to directly patch the function to return an empty struct with just { NULL },, that would mean the double check would not be necessary anymore and *cur would safely return NULL and stop the while without the need for the double check.
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
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.
Fixes the SIGSEGV reported in #650.
get_supported_devs()returnsNULLfor any vendor that is not IBM, HP, HPEor QUANTUM — the initialiser is
NULLand theswitchhas nodefault— andthe
sgandcambackends use the result without checking it. Opening a drivewhose INQUIRY vendor id is not one of those four therefore segfaults instead of
returning
-EDEV_DEVICE_UNSUPPORTABLEthree lines further down.The
iokitbackend already guards this withwhile(cur && *cur)(
iokit_tape.c:861); this makes the other two agree with it. No behaviourchanges for any supported drive: the added condition is only ever false when
the table is NULL, which is exactly the unsupported case.
iokit_tape.c:1000has a similar loop but walksibm_supported_drivesdirectly, which is never NULL, so it is left alone.
Verified
Rocky Linux 9.8, LTFS built from
v2.4.9.0-10523, drive presented by mhvtl asvendor
STK, productULT3580-TD8, no cartridge loaded:ltfs -o devname=/dev/sg9 /tmp/probeLTFS30209I Opening a device through sg-ibmtape driverLTFS30213I Unsupported Drive 'STK ' / 'ULT3580-TD8 '., exit 1Base is
release/v2.4.9.1, following the process in CONTRIBUTING — the recentfixes I could see all went to the active release branch rather than
main. Saythe word if you would rather have it elsewhere.
Not verified
The
cam(FreeBSD) change is the identical expression in the identicalposition and is not compiled or run in my environment. Happy to drop it
from this PR if you would rather it went separately.