Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] Missing DRM device removal protection (drm_dev_enter/exit) in IOCTL handlers leads to a Use-After-Free of the RPMsg endpoint. - [High] A shared completion mechanism without transaction IDs causes delayed hardware ACKs to falsely complete subsequent IOCTL requests. - [High] Spurious or early RPMsg messages cause a NULL pointer dereference in `npac_cb` before `probe` completes. - [High] The ADD and DELETE rule IOCTLs send identical payloads over RPMsg without any command identifier. - [Medium] Implicit padding in the `npac_acl_offload_rule` UAPI structure leaks uninitialized userspace memory to the remote processor and breaks ABI. - [Medium] Driver ignores the remote processor's response content, incorrectly assuming all ACKs indicate success. - [Low] UAPI structs and IOCTL definitions are placed in the driver C file instead of a dedicated UAPI header. --
commit f09e8a02272732ea035f9ef6c3a3f177ff86129a Author: Jaspinder Budhal <[email protected]> accel/npac: Add NPAC client driver This commit adds the Linux client driver for the Network Packet ACcelerator (NPAC). The driver communicates with the NPAC host driver via RPMsg to offload ethernet-related packet processing tasks, providing an IOCTL interface for userspace ACL rule management. > diff --git a/drivers/accel/npac/npac_drv.c b/drivers/accel/npac/npac_drv.c > new file mode 100644 > index 0000000000000..7516382ba74b6 > --- /dev/null > +++ b/drivers/accel/npac/npac_drv.c [ ... ] > +struct npac_acl_offload_rule { > + __u32 match_type; > + __u32 verdict; > + __u8 match_value[NPAC_ACL_MATCH_VALUE_MAX]; > +}; [Severity: Medium] Does this structure have an implicit 2-byte padding at the end due to the __u32 alignment? If so, the DRM IOCTL copy_from_user() path could copy uninitialized memory from userspace, leaking it to the remote processor. Should explicit padding (e.g., __u8 reserved[2]) be added to stabilize the ABI? [Severity: High] How does the hardware differentiate between ADD and DELETE commands? Since both IOCTLs send this exact same payload, the remote processor receives an identical byte sequence without an opcode or command identifier. > +#define DRM_IOCTL_NPAC_ACL_ADD_RULE \ > + DRM_IOWR(DRM_COMMAND_BASE + NPAC_ACL_ADD_RULE, struct > npac_acl_offload_rule) > + > +#define DRM_IOCTL_NPAC_ACL_DELETE_RULE \ > + DRM_IOWR(DRM_COMMAND_BASE + NPAC_ACL_DELETE_RULE, struct > npac_acl_offload_rule) [Severity: Low] Should these UAPI structures and IOCTL definitions be placed in a dedicated UAPI header (e.g., include/uapi/drm/npac_drm.h) instead of being hidden in the driver source? [ ... ] > +static int npac_add_rule_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file) > +{ > + struct npac_device *npac = drm_to_npac_device(dev); > + struct npac_acl_offload_rule *rule = data; > + int ret; > + > + dev_dbg(dev->dev, "add rule ioctl received\n"); > + > + mutex_lock(&npac->req_lock); > + reinit_completion(&npac->ack); > + ret = rpmsg_send(npac->rpdev->ept, rule, sizeof(*rule)); [Severity: High] Can this result in a use-after-free if the RPMsg device is unbound while the IOCTL is executing? Since the execution isn't protected by drm_dev_enter() and drm_dev_exit(), drm_dev_unplug() can return immediately and the RPMsg endpoint could be destroyed while rpmsg_send() is still accessing it. > + if (ret) { > + dev_err(dev->dev, "rpmsg_send failed: %d\n", ret); > + mutex_unlock(&npac->req_lock); > + return ret; > + } > + if (!wait_for_completion_timeout(&npac->ack, > + > msecs_to_jiffies(NPAC_ACK_TIMEOUT_MS))) { [Severity: High] If this wait times out, the firmware might still be processing the command and send an ACK later. If a subsequent IOCTL acquires req_lock and calls reinit_completion(), could a delayed ACK from the first request cause the second IOCTL to falsely return success before its actual request is processed? Should there be a transaction ID to match ACKs to specific requests? [ ... ] > +static int npac_cb(struct rpmsg_device *rpdev, void *data, int len, > + void *priv, u32 src) > +{ > + struct npac_device *npac = dev_get_drvdata(&rpdev->dev); > + > + dev_dbg(&rpdev->dev, "ack received\n"); > + complete(&npac->ack); [Severity: High] Is it possible for dev_get_drvdata() to return NULL here? If a message arrives on the RPMsg bus before dev_set_drvdata() is called during npac_probe(), this will result in a NULL pointer dereference. [Severity: Medium] Does the remote processor ever send status or error codes in the payload if a rule is rejected (e.g., if the hardware rule table is full)? The data and len parameters are completely ignored here, which might cause userspace to believe a rule was successfully offloaded when the hardware actually rejected it. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
