intel · pfl · Sep 9, 2026 · Sep 9, 2026
@@ -43,6 +43,10 @@ authorise the heavier one.
 Recovery Jobs run `xpu-smi` from the image in `spec.xpuSmi`, privileged, pinned to the affected node,
 and are retained after completion for post-mortem diagnostics (they are removed when the event is).
 
+The command is run as `/bin/sh -c "xpu-smi …"`, so an image supplied through `spec.xpuSmi.image` has
+to provide a shell and have `xpu-smi` on `PATH`. The operator deliberately does not assume an install
+path: where the binary lives is the image's business.
+
 ### Reset types
 
 | Type | Command | Notes |

@@ -6,6 +6,10 @@
 # The operator fills in: spec.template.spec.nodeName, containers[resetter].image,
 # and containers[resetter].args with the appropriate xpu-smi reset command.
 #
+# The command is /bin/sh -c and the reset is a single command line in args, so
+# xpu-smi is resolved on PATH: where the image keeps its binary is the image's
+# business, not the operator's.
+#
 # Supported commands (set by the operator from the event's recoveryType):
 #   sbr:   xpu-smi config -d <BDF> --reset
 #   slot:  xpu-smi config -d <BDF> --coldreset
@@ -39,12 +43,10 @@ spec:
       - name: resetter
         image: intel/gpu-fwupdater-mock:devel # replaced by the operator (spec.xpuSmi.image)
         imagePullPolicy: IfNotPresent
-        command: [ "/usr/local/bin/xpu-smi" ]
+        command: [ "/bin/sh", "-c" ]
         args:
-          - "config"
-          - "-d"
-          - "0000:00:00.0"  # replaced by the operator
-          - "--reset"       # replaced by the operator
+          # Replaced by the operator with the reset the event calls for.
+          - "xpu-smi config -d 0000:00:00.0 --reset"
         resources:
           requests:
             cpu: 50m

@@ -332,6 +332,17 @@ func (r *GPURecoveryPlanReconciler) syncRecoveryEventsFromSlices(ctx context.Con
 				continue
 			}
 
+			// The attribute is a free-form string the DRA driver writes, and it reaches the shell
+			// command line of a privileged root container. A device the operator cannot name a
+			// PCI address for is one it cannot recover either way, so the shape is required here
+			// rather than escaped later.
+			if !validDeviceBDF(bdf) {
+				klog.Warningf("ResourceSlice %s device %s has %s %q, which is not a PCI address, skipping",
+					slice.Name, dev.Name, deviceAttrBDF, bdf)
+
+				continue
+			}
+
 			key := deviceKey{node: nodeName, bdf: bdf}
 
 			for _, taint := range dev.Taints {
@@ -983,8 +994,8 @@ func (r *GPURecoveryPlanReconciler) createRecoveryJob(ctx context.Context, plan
 func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) error {
 	rt := evt.RecoveryType.Type
 
-	args := recoveryTypeToArgs(evt.GPUBDF, rt)
-	if args == nil {
+	cmd := buildResetCommand(evt.GPUBDF, rt)
+	if cmd == "" {
 		klog.Warningf("GPURecoveryPlan %s: unsupported recovery type %s for event %s; skipping", plan.Name, rt, evt.ID)
 
 		return nil
@@ -993,10 +1004,13 @@ func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *in
 	job := deployments.XpuManagerResetJob()
 	jobName := r.prepareRecoveryJob(job, plan, evt)
 
-	// Inject the xpu-smi image and the reset command from the plan and the event.
+	// Inject the xpu-smi image and the reset command from the plan and the event. The template's
+	// command is /bin/sh -c, as the reflash template's is, so the reset is one argument: a command
+	// line, not an argv. Going through a shell is what lets xpu-smi be found on PATH, rather than
+	// the operator having to know where the image the plan names keeps its binary.
 	if c := containerByName(job.Spec.Template.Spec.Containers, resetJobContainer); c != nil {
 		applyXpuSmiImage(c, plan)
-		c.Args = args
+		c.Args = []string{cmd}
 	}
 
 	if err := r.Create(ctx, job); err != nil {
@@ -1058,8 +1072,8 @@ func (r *GPURecoveryPlanReconciler) createReflashJob(ctx context.Context, plan *
 
 		// The template's command is /bin/sh -c, so the flash is one argument: a command line, not an
 		// argv. Overwriting args rather than command keeps the shell, which the template needs
-		// anyway.
-		c.Args = buildFDOFlashCommand(evt.GPUBDF, fw.File)
+		// anyway, and leaves xpu-smi to be found on PATH inside whichever image the plan names.
+		c.Args = []string{buildFDOFlashCommand(evt.GPUBDF, fw.File)}
 	}
 
 	if err := r.Create(ctx, job); err != nil {

@@ -860,6 +860,46 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 			Expect(r.syncRecoveryEventsFromSlices(ctx, p)).To(Succeed())
 			Expect(p.Status.Events).To(BeEmpty())
 		})
+
+		// A BDF that is not a PCI address is dropped at the same point, and for a sharper reason:
+		// it would otherwise be interpolated into the command line of a privileged root container.
+		// Detection is the only place the value enters the plan, so this is what lets both command
+		// builders interpolate it without escaping.
+		It("should skip a tainted device whose pciAddress is not a PCI address", func() {
+			slice := &resv1.ResourceSlice{
+				ObjectMeta: metav1.ObjectMeta{Name: "slice-bad-bdf"},
+				Spec: resv1.ResourceSliceSpec{
+					Driver:   "gpu.intel.com",
+					NodeName: ptr.To("node-bad-bdf"),
+					Pool:     resv1.ResourcePool{Name: "pool-bad-bdf", ResourceSliceCount: 1},
+					Devices: []resv1.Device{{
+						Name: "dev-bad-bdf",
+						Attributes: map[resv1.QualifiedName]resv1.DeviceAttribute{
+							deviceAttrDeviceID: {StringValue: ptr.To("0xabcd")},
+							deviceAttrBDF:      {StringValue: ptr.To("0000:02:00.0; touch /tmp/pwned")},
+						},
+						Taints: []resv1.DeviceTaint{
+							{Key: deviceTaintKeyReset, Effect: resv1.DeviceTaintEffectNoSchedule},
+						},
+					}},
+				},
+			}
+			Expect(k8sClient.Create(ctx, slice)).To(Succeed())
+			DeferCleanup(func() {
+				_ = k8sClient.Delete(ctx, slice)
+			})
+
+			r := newTestReconciler()
+			p := &intelv1a1.GPURecoveryPlan{
+				ObjectMeta: metav1.ObjectMeta{Name: "plan-bad-bdf"},
+				Spec: intelv1a1.GPURecoveryPlanSpec{
+					DeviceID: "0xabcd", DefaultResetType: intelv1a1.RecoveryTypeSlot,
+				},
+			}
+
+			Expect(r.syncRecoveryEventsFromSlices(ctx, p)).To(Succeed())
+			Expect(p.Status.Events).To(BeEmpty())
+		})
 	})
 
 	Context("Helper: taintToDeviceNeed", func() {
@@ -2231,8 +2271,8 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 
 			Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeSlot))
 			Expect(evt.RecoveryType.SuggestedType).To(Equal(intelv1a1.RecoveryTypeSBR))
-			Expect(recoveryTypeToArgs("0000:02:00.0", evt.RecoveryType.Type)).To(
-				ContainElement("--coldreset"), "an override must change the command actually run")
+			Expect(buildResetCommand("0000:02:00.0", evt.RecoveryType.Type)).To(
+				ContainSubstring("--coldreset"), "an override must change the command actually run")
 		})
 
 		It("should be a no-op when the approval has no override", func() {
@@ -2816,7 +2856,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 		})
 	})
 
-	Context("Helper: recoveryTypeToArgs", func() {
+	Context("Helper: buildResetCommand", func() {
 		const bdf = "0000:02:00.0"
 
 		// Every reset type in the enum must map to a distinct xpu-smi invocation, or an admin's
@@ -2831,32 +2871,75 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 			seen := map[string]intelv1a1.RecoveryType{}
 
 			for _, rt := range resetTypes {
-				args := recoveryTypeToArgs(bdf, rt)
-				Expect(args).NotTo(BeEmpty(), "reset type %q must map to a command", rt)
+				cmd := buildResetCommand(bdf, rt)
+				Expect(cmd).NotTo(BeEmpty(), "reset type %q must map to a command", rt)
 
-				key := strings.Join(args, " ")
-				Expect(seen).NotTo(HaveKey(key),
+				Expect(seen).NotTo(HaveKey(cmd),
 					"reset types %q and %q share the command %q, so an override between them is a no-op",
-					seen[key], rt, key)
+					seen[cmd], rt, cmd)
 
-				seen[key] = rt
+				seen[cmd] = rt
 			}
 		})
 
+		// The binary is named, not pathed: the plan can point spec.xpuSmi.image at any image that
+		// has xpu-smi on PATH, which is not the same set of images as those that keep it in
+		// /usr/local/bin.
+		It("should invoke xpu-smi by name rather than by path", func() {
+			Expect(buildResetCommand(bdf, intelv1a1.RecoveryTypeSlot)).To(HavePrefix("xpu-smi "))
+		})
+
 		It("should address the BDF the event names", func() {
-			Expect(recoveryTypeToArgs("0000:af:00.0", intelv1a1.RecoveryTypeSBR)).
-				To(ContainElement("0000:af:00.0"))
+			Expect(buildResetCommand("0000:af:00.0", intelv1a1.RecoveryTypeSBR)).
+				To(ContainSubstring("0000:af:00.0"))
 		})
 
-		It("should return nil for reflash, which is not an xpu-smi reset", func() {
-			Expect(recoveryTypeToArgs(bdf, intelv1a1.RecoveryTypeReflash)).To(BeNil())
+		It("should return no command for reflash, which is not an xpu-smi reset", func() {
+			Expect(buildResetCommand(bdf, intelv1a1.RecoveryTypeReflash)).To(BeEmpty())
 		})
 
-		It("should return nil for a type outside the enum", func() {
-			Expect(recoveryTypeToArgs(bdf, intelv1a1.RecoveryType("flr"))).To(BeNil())
+		It("should return no command for a type outside the enum", func() {
+			Expect(buildResetCommand(bdf, intelv1a1.RecoveryType("flr"))).To(BeEmpty())
 		})
 	})
 
+	// Both command lines interpolate the BDF into a string /bin/sh parses, so the shape of the
+	// ResourceSlice attribute they come from is what stands between a free-form driver-written
+	// string and a privileged root shell.
+	Context("Helper: validDeviceBDF", func() {
+		DescribeTable("should accept the addresses the kernel prints",
+			func(bdf string) {
+				Expect(validDeviceBDF(bdf)).To(BeTrue(), "%q is a PCI address", bdf)
+			},
+			Entry("domain zero", "0000:02:00.0"),
+			Entry("non-zero domain", "0001:af:00.0"),
+			// lspci prints PCI addresses in uppercase hex, so this is a likely spelling rather
+			// than a pathological one.
+			Entry("uppercase hex", "0000:AF:00.0"),
+			Entry("highest function", "0000:00:1f.7"),
+		)
+
+		DescribeTable("should reject anything else",
+			func(bdf string) {
+				Expect(validDeviceBDF(bdf)).To(BeFalse(), "%q is not a PCI address", bdf)
+			},
+			Entry("empty", ""),
+			Entry("no domain", "02:00.0"),
+			Entry("no function", "0000:02:00"),
+			Entry("function out of range", "0000:02:00.8"),
+			Entry("non-hex digits", "0000:0g:00.0"),
+			Entry("unexpected separators", "0000/02_00,0"),
+			Entry("trailing separator", "0000:02:00.0:"),
+			Entry("leading separator", ":02:00.0"),
+			Entry("trailing newline", "0000:02:00.0\n"),
+			// The one that matters: a shell metacharacter must not reach a command line run as
+			// root in a privileged container.
+			Entry("shell command substitution", "0000:02:00.0; touch /tmp/pwned"),
+			Entry("shell pipeline", "0000:02:00.0 | sh"),
+			Entry("backticks", "`id`"),
+		)
+	})
+
 	// The deadline is the only clock running once a recovery Job exists: nothing in the reconcile
 	// gives up on an in-progress event, so an xpu-smi that hangs on a card that has stopped answering
 	// would hold the node's drain taint and the event's state indefinitely.
@@ -3190,8 +3273,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 			Expect(resetter).NotTo(BeNil())
 			Expect(resetter.Image).To(Equal("registry/xpu-smi:v1"))
 			Expect(resetter.ImagePullPolicy).To(Equal(core.PullAlways))
-			// The BDF has to reach the command line, not just the event.
-			Expect(resetter.Args).To(Equal([]string{"config", "-d", "0000:02:00.0", "--coldreset"}))
+			// The BDF has to reach the command line, not just the event. One argument, because the
+			// template runs /bin/sh -c: see the reflash Job's specs for what splitting it costs.
+			Expect(resetter.Command).To(Equal([]string{"/bin/sh", "-c"}))
+			Expect(resetter.Args).To(Equal([]string{"xpu-smi config -d 0000:02:00.0 --coldreset"}))
 		})
 
 		It("should give the Job the operator's own pull secret", func() {
@@ -3264,6 +3349,28 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 		})
 	})
 
+	// Both createResetJob and createReflashJob hand their container exactly one argument, so both
+	// templates have to keep a shell as their command. This is a contract between Go and YAML that
+	// nothing else checks: a template switched back to running the binary directly would exec a whole
+	// command line as argv[0] and fail inside a pod with a "no such file" naming the entire string.
+	//
+	// Going through a shell is also what keeps the path to xpu-smi out of the operator. The plan names
+	// an image; where that image keeps its binary is its own business, so the command line invokes
+	// xpu-smi by name and lets PATH resolve it.
+	Context("Recovery Job templates: the shell contract", func() {
+		DescribeTable("should run xpu-smi through a shell, one command line at a time",
+			func(job *batch.Job, containerName string) {
+				c := containerByName(job.Spec.Template.Spec.Containers, containerName)
+				Expect(c).NotTo(BeNil())
+				Expect(c.Command).To(Equal([]string{"/bin/sh", "-c"}))
+				Expect(c.Args).To(HaveLen(1),
+					"the template's own args stand in for what the operator writes, so they must be one command line too")
+			},
+			Entry("reset", deployments.XpuManagerResetJob(), resetJobContainer),
+			Entry("reflash", deployments.XpuManagerFWUpdateJob(), reflashJobContainer),
+		)
+	})
+
 	// A card in survivability mode has firmware that no reset can fix, so the recovery is to write a
 	// known-good image over it. That is a different Job from a reset — a firmware image, an
 	// initContainer that stages it, and a shell command line rather than an argv — and every part the
@@ -3359,9 +3466,13 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 			Expect(flashC.ImagePullPolicy).To(Equal(core.PullAlways))
 
 			// The template runs /bin/sh -c, so the flash has to stay one command line: replacing the
-			// command with an argv, as a reset does, would leave the shell nothing to run.
+			// command with an argv, as a reset does, would leave the shell nothing to run. The length
+			// is the assertion that matters — sh -c takes its command from the first operand and turns
+			// the rest into $0, $1, …, so a flash split across arguments runs a bare "xpu-smi" and
+			// silently flashes nothing.
 			Expect(flashC.Command).To(Equal([]string{"/bin/sh", "-c"}))
-			Expect(flashC.Args).To(Equal(buildFDOFlashCommand(reflashBDF, reflashFile)))
+			Expect(flashC.Args).To(HaveLen(1))
+			Expect(flashC.Args[0]).To(Equal(buildFDOFlashCommand(reflashBDF, reflashFile)))
 
 			// An admin who has to check what was flashed onto which card reads this, not the pod log.
 			Expect(p.Status.Messages).To(ContainElement(SatisfyAll(
@@ -3403,7 +3514,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 			Expect(mountedAt(copyC, reflashStagingDir)).NotTo(BeEmpty())
 			Expect(mountedAt(flashC, reflashStagingDir)).To(Equal(mountedAt(copyC, reflashStagingDir)))
 
-			Expect(strings.Join(buildFDOFlashCommand(reflashBDF, reflashFile), " ")).To(
+			Expect(buildFDOFlashCommand(reflashBDF, reflashFile)).To(
 				ContainSubstring(reflashStagingDir + "/" + reflashFile))
 			Expect(firmwareImagePath(reflashFile)).To(Equal(firmwareImageDir + "/" + reflashFile))
 		})
@@ -3412,7 +3523,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() {
 		// survivability mode: xpu-smi otherwise declines to write an image it judges no newer than
 		// what is on the device, and what is on the device is exactly what has to go.
 		It("should force an unattended FDO flash", func() {
-			Expect(strings.Join(buildFDOFlashCommand("0000:02:00.0", "fw.bin"), " ")).To(
+			Expect(buildFDOFlashCommand("0000:02:00.0", "fw.bin")).To(
 				Equal("xpu-smi updatefw -d 0000:02:00.0 -t FDO -f /update/fw.bin -y --force"))
 		})
 

@@ -21,6 +21,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"regexp"
 	"strings"
 	"time"
 	"unicode/utf8"
@@ -219,26 +220,54 @@ func hashSegment(s string) string {
 	return hex.EncodeToString(sum[:])[:idHashLen]
 }
 
-// recoveryTypeToArgs returns the xpu-smi command-line arguments that carry out the given reset
-// against the given BDF. Returns nil for a type xpu-smi has no reset for, reflash above all: it
-// writes firmware rather than resetting the device, so it is a different Job entirely.
-func recoveryTypeToArgs(bdf string, rt intelv1a1.RecoveryType) []string {
+// deviceBDFPattern is the shape the kernel prints a PCI address in ("%04x:%02x:%02x.%d", function
+// 0-7), accepting either hex case because lspci and some drivers spell the bus uppercase.
+//
+// Both recovery command lines interpolate the BDF into a string a privileged root container hands
+// to /bin/sh, and it arrives from a free-form ResourceSlice attribute rather than from a validated
+// API field, so it is checked on the way in — see validDeviceBDF's callers.
+var deviceBDFPattern = regexp.MustCompile(`^[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}\.[0-7]$`)
+
+// validDeviceBDF reports whether bdf is a PCI address the recovery Jobs can be handed.
+//
+// Deliberately an allow-list of the one shape that is ever correct, not a deny-list of shell
+// metacharacters, for the same reason firmwareFileNamePattern is: the value ends up in a command
+// line inside a privileged root container, where anything unanticipated is worse than a device the
+// operator declines to recover and says so about.
+func validDeviceBDF(bdf string) bool {
+	return deviceBDFPattern.MatchString(bdf)
+}
+
+// buildResetCommand returns the shell command line that carries out the given reset against the
+// given BDF. Returns "" for a type xpu-smi has no reset for, reflash above all: it writes firmware
+// rather than resetting the device, so it is a different Job entirely.
+//
+// One string rather than an argv, as buildFDOFlashCommand is and for the same reason — see there.
+func buildResetCommand(bdf string, rt intelv1a1.RecoveryType) string {
 	switch rt {
 	case intelv1a1.RecoveryTypeSBR:
-		return []string{"config", "-d", bdf, "--reset"}
+		return fmt.Sprintf("xpu-smi config -d %s --reset", bdf)
 	case intelv1a1.RecoveryTypeSlot:
-		return []string{"config", "-d", bdf, "--coldreset"}
+		return fmt.Sprintf("xpu-smi config -d %s --coldreset", bdf)
 	case intelv1a1.RecoveryTypeAMC:
-		return []string{"amc", "--gpureset", "-d", bdf, "-y"}
+		return fmt.Sprintf("xpu-smi amc --gpureset -d %s -y", bdf)
 	default:
-		return nil
+		return ""
 	}
 }
 
 // buildFDOFlashCommand returns the shell command line that reflashes one GPU from a firmware file the
 // fw-copy initContainer has staged in the shared volume.
-func buildFDOFlashCommand(bdf, file string) []string {
-	return []string{"xpu-smi", "updatefw", "-d", bdf, "-t", "FDO", "-f", fmt.Sprintf("%s/%s", reflashStagingDir, file), "-y", "--force"}
+//
+// One string, not an argv: the container it lands on runs /bin/sh -c, which takes the command from
+// its first operand only and turns the rest into positional parameters. As an argv this flashed
+// nothing — the shell ran a bare "xpu-smi" and everything after it became $0, $1 and so on.
+//
+// Both interpolated values are shape-checked before they can reach here: the webhook allow-lists
+// spec.firmware.file down to [a-zA-Z0-9._-] (firmwareFileNamePattern) and detection drops any device
+// whose PCI address is not a BDF (validDeviceBDF).
+func buildFDOFlashCommand(bdf, file string) string {
+	return fmt.Sprintf("xpu-smi updatefw -d %s -t FDO -f %s/%s -y --force", bdf, reflashStagingDir, file)
 }
 
 // firmwareImagePath returns path to the firmware file inside the firmware image.