[PATCH] drm/xe/pf: clear pending packet on header init failure

[PATCH] drm/xe/pf: clear pending packet on header init failure

[Guangshuo Li]

xe_sriov_packet_write_single() keeps a partially written restore packet
in the per-VF pending slot across write() calls.

When the header becomes complete, xe_sriov_packet_init_from_hdr() sets
data->remaining from the user supplied header before initializing the
packet backing storage. If that initialization fails, for example because
the header contains an invalid gt_id, pkt_hdr_write() returns an error
but the pending slot is left pointing at the partially initialized packet.

A later write then reuses that packet, skips the header path because
hdr_remaining is already zero, and reaches pkt_data_write() without
backing storage installed. This can dereference data->vaddr while it is
still NULL.

Drop the pending packet when header initialization fails after the header
has been fully consumed, so the next write starts from a fresh packet.

Fixes: 1ed30397c0b9 ("drm/xe/pf: Add support for encap/decap of bitstream to/from packet")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/drm/xe/xe_sriov_packet.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_sriov_packet.c b/drivers/gpu/drm/xe/xe_sriov_packet.c
index 2ae9eff2a7c0..e9c92019862e 100644
--- a/drivers/gpu/drm/xe/xe_sriov_packet.c
+++ b/drivers/gpu/drm/xe/xe_sriov_packet.c
@@ -323,8 +323,9 @@ ssize_t xe_sriov_packet_write_single(struct xe_device *xe, unsigned int vfid,
 				     const char __user *buf, size_t len)
 {
 	struct xe_sriov_packet **data = pf_pick_pending(xe, vfid);
-	int ret;
 	ssize_t copied;
+	bool writing_hdr;
+	int ret;
 
 	if (IS_ERR_OR_NULL(*data)) {
 		*data = xe_sriov_packet_alloc(xe);
@@ -332,11 +333,21 @@ ssize_t xe_sriov_packet_write_single(struct xe_device *xe, unsigned int vfid,
 			return -ENOMEM;
 	}
 
-	if ((*data)->hdr_remaining)
+	writing_hdr = (*data)->hdr_remaining;
+	if (writing_hdr)
 		copied = pkt_hdr_write(*data, buf, len);
 	else
 		copied = pkt_data_write(*data, buf, len);
 
+	if (copied < 0) {
+		if (writing_hdr && !(*data)->hdr_remaining) {
+			kfree(*data);
+			*data = NULL;
+		}
+
+		rturn copied;
+	}
+
 	if ((*data)->hdr_remaining == 0 && (*data)->remaining == 0) {
 		ret = xe_sriov_pf_migration_restore_produce(xe, vfid, *data);
 		if (ret) {
-- 
2.43.0

Re: [PATCH] drm/xe/pf: clear pending packet on header init failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch introduces a syntax error `rturn copied;` instead of `return copied;`.

Pre-existing issues:
- [High] Memory leak of migration packets across aborted or repeated SAVE operations.
--

--- Patch [1]: [PATCH] drm/xe/pf: clear pending packet on header init failure ---
commit c2701f46b2a82f4a8b22c425cd68c28c08222f7f
Author: Guangshuo Li <lgs201920130244@gmail.com>

drm/xe/pf: clear pending packet on header init failure

This commit drops the pending packet when header initialization fails
after the header has been fully consumed. This prevents a potential
NULL pointer dereference on subsequent writes.

> diff --git a/drivers/gpu/drm/xe/xe_sriov_packet.c b/drivers/gpu/drm/xe/xe_sriov_packet.c
> --- a/drivers/gpu/drm/xe/xe_sriov_packet.c
> +++ b/drivers/gpu/drm/xe/xe_sriov_packet.c
[ ... ]
> @@ -332,11 +333,21 @@ ssize_t xe_sriov_packet_write_single(struct xe_device *xe, unsigned int vfid,
> -	if ((*data)->hdr_remaining)
> +	writing_hdr = (*data)->hdr_remaining;
> +	if (writing_hdr)
>  		copied = pkt_hdr_write(*data, buf, len);
>  	else
>  		copied = pkt_data_write(*data, buf, len);
>  
> +	if (copied < 0) {
> +		if (writing_hdr && !(*data)->hdr_remaining) {
> +			kfree(*data);
> +			*data = NULL;
> +		}
> +
> +		rturn copied;

[Severity: Low]
There is a typo here (rturn instead of return).

> +	}
> +
>  	if ((*data)->hdr_remaining == 0 && (*data)->remaining == 0) {
>  		ret = xe_sriov_pf_migration_restore_produce(xe, vfid, *data);

[Severity: High]
This is a pre-existing issue, but is it possible to leak migration packets
across aborted or repeated save operations?

If userspace initiates a VF migration save operation, aborts it without
reading all packets, and then triggers a new save, will the pointers be
overwritten without freeing the previously allocated packets?

drivers/gpu/drm/xe/xe_sriov_packet.c:pf_descriptor_init() {
    ...
    *desc = data;
    ...
}

drivers/gpu/drm/xe/xe_sriov_packet.c:pf_trailer_init() {
    ...
    *trailer = data;
    ...
}

drivers/gpu/drm/xe/xe_sriov_packet.c:pf_pending_init() {
    ...
    *data = NULL;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708071502.722393-1-lgs201920130244@gmail.com?part=1

✗ LGCI.VerificationFailed: failure for drm/xe/pf: clear pending packet on header init failure

[Patchwork]

== Series Details ==

Series: drm/xe/pf: clear pending packet on header init failure
URL   : https://patchwork.freedesktop.org/series/170005/
State : failure

== Summary ==

Series author address 'lgs201920130244@gmail.com' is not on the allowlist, which prevents CI from being automatically triggered.
If you want CI to run for this series, ask Patchwork project owners to click 'retest' on the series in Patchwork.
Exception occurred during validation, bailing out!
Build URL: http://intel-gfx-ci-public.igk.intel.com:8080/job/xe_pw_trigger/1209615/ (on master)