[PATCH net] net: qualcomm: rmnet: avoid stale VND access on TX drop

[PATCH net] net: qualcomm: rmnet: avoid stale VND access on TX drop

[Jérémy Jean]

A struct rmnet_port stores a pointer to the most recently created
RMNET VND in port->rmnet_dev, but userspace can delete that VND while
the port still has another attached VND. When RMNET later sends a
buffered TX skb, a failure in __skb_linearize() makes rmnet_send_skb()
update the drop counter through that stale pointer, which causes a UAF.

Account TX drops on the lower device instead of port->rmnet_dev.

Fixes: 64b5d1f8f2d1 ("net: qualcomm: rmnet: add tx packets aggregation")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index e6f48dc9fe64..4df75f4215fa 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -543,10 +543,7 @@ static void rmnet_send_skb(struct rmnet_port *port, struct sk_buff *skb)
 {
 	if (skb_needs_linearize(skb, port->dev->features)) {
 		if (unlikely(__skb_linearize(skb))) {
-			struct rmnet_priv *priv;
-
-			priv = netdev_priv(port->rmnet_dev);
-			this_cpu_inc(priv->pcpu_stats->stats.tx_drops);
+			dev_core_stats_tx_dropped_inc(port->dev);
 			dev_kfree_skb_any(skb);
 			return;
 		}
-- 
2.47.3

[PATCH net] net: qualcomm: rmnet: bound TX aggregates on byte-limit update

[Jérémy Jean]

If userspace reduces the TX aggregation byte limit while a buffered skb
is pending, subtracting the skb length from the new limit underflows.
The later comparison with unsigned skb->len then lets the buffered skb
grow past the limit.

Check lengths before subtracting. Flush a buffered skb if it is already
at or above the new byte limit.

Fixes: 64b5d1f8f2d1 ("net: qualcomm: rmnet: add tx packets aggregation")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index e6f48dc9fe64..205ac81f2c82 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -594,7 +594,7 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
 	struct timespec64 diff, last;
 	unsigned int len = skb->len;
 	struct sk_buff *agg_skb;
-	int size;
+	unsigned int size;
 
 	spin_lock_bh(&port->agg_lock);
 	memcpy(&last, &port->agg_last, sizeof(struct timespec64));
@@ -606,13 +606,14 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
 		 */
 new_packet:
 		diff = timespec64_sub(port->agg_last, last);
-		size = port->egress_agg_params.bytes - skb->len;
+		size = port->egress_agg_params.bytes;
 
-		if (size < 0) {
+		if (skb->len > size) {
 			/* dropped */
 			spin_unlock_bh(&port->agg_lock);
 			return 0;
 		}
+		size -= skb->len;
 
 		if (diff.tv_sec > 0 || diff.tv_nsec > RMNET_AGG_BYPASS_TIME_NSEC ||
 		    size == 0)
@@ -630,9 +631,8 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
 		goto schedule;
 	}
 	diff = timespec64_sub(port->agg_last, port->agg_time);
-	size = port->egress_agg_params.bytes - port->skbagg_head->len;
-
-	if (skb->len > size) {
+	if (port->skbagg_head->len >= port->egress_agg_params.bytes ||
+	    skb->len > port->egress_agg_params.bytes - port->skbagg_head->len) {
 		agg_skb = port->skbagg_head;
 		reset_aggr_params(port);
 		spin_unlock_bh(&port->agg_lock);
-- 
2.47.3

Re: [PATCH net] net: qualcomm: rmnet: bound TX aggregates on byte-limit update

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: qualcomm: rmnet: avoid stale VND access on TX drop

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: qualcomm: rmnet: avoid stale VND access on TX drop

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete: the stale port->rmnet_dev UAF described
  in the commit message can still be reached.

Pre-existing issues:
- [High] Teardown race: rmnet_port can be freed while its hrtimer is
  still armed.

--

net: qualcomm: rmnet: avoid stale VND access on TX drop

This commit changes rmnet_send_skb() so that a TX drop after a failed
__skb_linearize() is counted on the lower device with
dev_core_stats_tx_dropped_inc(). Before, the drop was counted through
port->rmnet_dev, which can still point at a VND that has been deleted.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 39d6d084e73f0..bd68d60b50d35 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -543,10 +543,7 @@ static void rmnet_send_skb(struct rmnet_port *port, struct sk_buff *skb)
>  {
>  	if (skb_needs_linearize(skb, port->dev->features)) {
>  		if (unlikely(__skb_linearize(skb))) {
> -			struct rmnet_priv *priv;
> -
> -			priv = netdev_priv(port->rmnet_dev);
> -			this_cpu_inc(priv->pcpu_stats->stats.tx_drops);

[Severity: High]
This removes one reader of the stale port->rmnet_dev. Can the stale
pointer still be reached another way?

rmnet_newlink() is the only function that writes port->rmnet_dev.
rmnet_dellink() does not clear or reassign it when the VND it points to
is deleted. rmnet_unregister_bridge() still uses it on the real-device
branch:

drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_unregister_bridge() {
    ...
	rmnet_dev = port->rmnet_dev;
	if (!port->nr_rmnet_devs) {
    ...
	} else {
		/* real device */
		bridge_dev = port->bridge_ep;
    ...
	}

	netdev_upper_dev_unlink(bridge_dev, rmnet_dev);
    ...
}

This sequence seems to reach it without any race:

1. Create VNDs A and B on real device R, so port->rmnet_dev = B.

2. Delete B. nr_rmnet_devs drops to 1 and B is unregistered and freed
   (needs_free_netdev), but port->rmnet_dev still points at B.

3. Run "ip link set S master A". rmnet_add_bridge() only rejects
   nr_rmnet_devs > 1, so the call succeeds. It puts R's port into
   bridge mode but does not update R's port->rmnet_dev.

4. Unregister R:

   rmnet_config_notify_cb(NETDEV_UNREGISTER)
     rmnet_force_unassociate_device(R)
       rmnet_unregister_bridge(port)
         netdev_upper_dev_unlink(S, B)   <- B already freed
           __netdev_upper_dev_unlink()

__netdev_upper_dev_unlink() passes B to the PRECHANGEUPPER and
CHANGEUPPER notifiers. It then calls __netdev_update_lower_level(B),
which writes to B, and __netdev_walk_all_upper_dev(B), which walks B's
adjacency lists.

The real S->A adjacency is also never removed. Does that leak the
reference on A, so that unregistering A later hangs in
netdev_wait_allrefs()?

Should port->rmnet_dev be kept valid in rmnet_dellink() instead? The
other option is to stop using it in rmnet_unregister_bridge() as well,
for example by using the slave port's rmnet_dev or
netdev_master_upper_dev_get(bridge_ep).

> +			dev_core_stats_tx_dropped_inc(port->dev);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
rmnet_send_skb() run on an rmnet_port that has already been freed? If
so, both port->dev->features and the new
dev_core_stats_tx_dropped_inc(port->dev) read freed memory.

When the last VND is deleted, rmnet_dellink() tears down the port before
it unregisters the VND:

	netdev_upper_dev_unlink(real_dev, dev);
	rmnet_unregister_real_device(real_dev);
	unregister_netdevice_queue(dev, head);

rmnet_unregister_real_device() then does:

	rmnet_map_tx_aggregate_exit(port);

	netdev_rx_handler_unregister(real_dev);

	kfree(port);

Nothing marks the port as shutting down. With TX aggregation enabled
(egress_agg_params.count > 1, set through ethtool coalesce), a transmit
on another CPU at the same time could go like this:

CPU1
rmnet_vnd_start_xmit()
  rmnet_egress_handler()
    port = rmnet_get_port_rcu(skb->dev);   <- before rx_handler is NULL
    rmnet_map_egress_handler()
      rmnet_map_tx_aggregate()
        spin_lock_bh(&port->agg_lock);     <- waits for CPU2

CPU2
rmnet_unregister_real_device()
  rmnet_map_tx_aggregate_exit()
    hrtimer_cancel(&port->hrtimer);
    cancel_work_sync(&port->agg_wq);
    spin_lock_bh(&port->agg_lock);
    ... frees skbagg_head ...
    spin_unlock_bh(&port->agg_lock);

CPU1 then takes agg_lock, sets port->skbagg_head with skb_copy_expand()
and re-arms the timer:

	hrtimer_start(&port->hrtimer,
		      ns_to_ktime(port->egress_agg_params.time_nsec),
		      HRTIMER_MODE_REL);

CPU2 continues:

  netdev_rx_handler_unregister()   <- synchronize_net() waits for CPU1
  kfree(port);                     <- hrtimer still queued

When the timer fires, rmnet_map_flush_tx_packet_queue() calls
schedule_work(&port->agg_wq) on the freed port. The work then reaches
rmnet_send_skb() through rmnet_map_flush_tx_packet_work(). The
aggregated skb also seems to leak.

synchronize_net() waits for the reader to finish, but nothing cancels
the timer that the reader armed. Would a shutdown flag checked under
agg_lock work here, or a second cancel after
netdev_rx_handler_unregister()?

This looks like it comes from the TX aggregation code added by the
commit in the Fixes: tag.

>  			dev_kfree_skb_any(skb);
>  			return;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031221052.1642262-1-Jeremy.Jean%40oss.cyber.gouv.fr

Re: [PATCH net] net: qualcomm: rmnet: avoid stale VND access on TX drop

[Jérémy Jean]

On 2026-11-01 23:11, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let 
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your 
> reply
> (one such reply per series is enough).
> 
> - [High] The fix is incomplete: the stale port->rmnet_dev UAF described
>   in the commit message can still be reached.
> 
> Pre-existing issues:
> - [High] Teardown race: rmnet_port can be freed while its hrtimer is
>   still armed.
> 
> --
> 
> net: qualcomm: rmnet: avoid stale VND access on TX drop
> 
> This commit changes rmnet_send_skb() so that a TX drop after a failed
> __skb_linearize() is counted on the lower device with
> dev_core_stats_tx_dropped_inc(). Before, the drop was counted through
> port->rmnet_dev, which can still point at a VND that has been deleted.
> 
>> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c 
>> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>> index 39d6d084e73f0..bd68d60b50d35 100644
>> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>> @@ -543,10 +543,7 @@ static void rmnet_send_skb(struct rmnet_port 
>> *port, struct sk_buff *skb)
>>  {
>>  	if (skb_needs_linearize(skb, port->dev->features)) {
>>  		if (unlikely(__skb_linearize(skb))) {
>> -			struct rmnet_priv *priv;
>> -
>> -			priv = netdev_priv(port->rmnet_dev);
>> -			this_cpu_inc(priv->pcpu_stats->stats.tx_drops);
> 
> [Severity: High]
> This removes one reader of the stale port->rmnet_dev. Can the stale
> pointer still be reached another way?
> 
> rmnet_newlink() is the only function that writes port->rmnet_dev.
> rmnet_dellink() does not clear or reassign it when the VND it points to
> is deleted. rmnet_unregister_bridge() still uses it on the real-device
> branch:
> 
> drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_unregister_bridge() 
> {
>     ...
> 	rmnet_dev = port->rmnet_dev;
> 	if (!port->nr_rmnet_devs) {
>     ...
> 	} else {
> 		/* real device */
> 		bridge_dev = port->bridge_ep;
>     ...
> 	}
> 
> 	netdev_upper_dev_unlink(bridge_dev, rmnet_dev);
>     ...
> }
> 
> This sequence seems to reach it without any race:
> 
> 1. Create VNDs A and B on real device R, so port->rmnet_dev = B.
> 
> 2. Delete B. nr_rmnet_devs drops to 1 and B is unregistered and freed
>    (needs_free_netdev), but port->rmnet_dev still points at B.
> 
> 3. Run "ip link set S master A". rmnet_add_bridge() only rejects
>    nr_rmnet_devs > 1, so the call succeeds. It puts R's port into
>    bridge mode but does not update R's port->rmnet_dev.
> 
> 4. Unregister R:
> 
>    rmnet_config_notify_cb(NETDEV_UNREGISTER)
>      rmnet_force_unassociate_device(R)
>        rmnet_unregister_bridge(port)
>          netdev_upper_dev_unlink(S, B)   <- B already freed
>            __netdev_upper_dev_unlink()
> 
> __netdev_upper_dev_unlink() passes B to the PRECHANGEUPPER and
> CHANGEUPPER notifiers. It then calls __netdev_update_lower_level(B),
> which writes to B, and __netdev_walk_all_upper_dev(B), which walks B's
> adjacency lists.
> 
> The real S->A adjacency is also never removed. Does that leak the
> reference on A, so that unregistering A later hangs in
> netdev_wait_allrefs()?
> 
> Should port->rmnet_dev be kept valid in rmnet_dellink() instead? The
> other option is to stop using it in rmnet_unregister_bridge() as well,
> for example by using the slave port's rmnet_dev or
> netdev_master_upper_dev_get(bridge_ep).
> 
>> +			dev_core_stats_tx_dropped_inc(port->dev);
> 
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. Can
> rmnet_send_skb() run on an rmnet_port that has already been freed? If
> so, both port->dev->features and the new
> dev_core_stats_tx_dropped_inc(port->dev) read freed memory.
> 
> When the last VND is deleted, rmnet_dellink() tears down the port 
> before
> it unregisters the VND:
> 
> 	netdev_upper_dev_unlink(real_dev, dev);
> 	rmnet_unregister_real_device(real_dev);
> 	unregister_netdevice_queue(dev, head);
> 
> rmnet_unregister_real_device() then does:
> 
> 	rmnet_map_tx_aggregate_exit(port);
> 
> 	netdev_rx_handler_unregister(real_dev);
> 
> 	kfree(port);
> 
> Nothing marks the port as shutting down. With TX aggregation enabled
> (egress_agg_params.count > 1, set through ethtool coalesce), a transmit
> on another CPU at the same time could go like this:
> 
> CPU1
> rmnet_vnd_start_xmit()
>   rmnet_egress_handler()
>     port = rmnet_get_port_rcu(skb->dev);   <- before rx_handler is NULL
>     rmnet_map_egress_handler()
>       rmnet_map_tx_aggregate()
>         spin_lock_bh(&port->agg_lock);     <- waits for CPU2
> 
> CPU2
> rmnet_unregister_real_device()
>   rmnet_map_tx_aggregate_exit()
>     hrtimer_cancel(&port->hrtimer);
>     cancel_work_sync(&port->agg_wq);
>     spin_lock_bh(&port->agg_lock);
>     ... frees skbagg_head ...
>     spin_unlock_bh(&port->agg_lock);
> 
> CPU1 then takes agg_lock, sets port->skbagg_head with skb_copy_expand()
> and re-arms the timer:
> 
> 	hrtimer_start(&port->hrtimer,
> 		      ns_to_ktime(port->egress_agg_params.time_nsec),
> 		      HRTIMER_MODE_REL);
> 
> CPU2 continues:
> 
>   netdev_rx_handler_unregister()   <- synchronize_net() waits for CPU1
>   kfree(port);                     <- hrtimer still queued
> 
> When the timer fires, rmnet_map_flush_tx_packet_queue() calls
> schedule_work(&port->agg_wq) on the freed port. The work then reaches
> rmnet_send_skb() through rmnet_map_flush_tx_packet_work(). The
> aggregated skb also seems to leak.
> 
> synchronize_net() waits for the reader to finish, but nothing cancels
> the timer that the reader armed. Would a shutdown flag checked under
> agg_lock work here, or a second cancel after
> netdev_rx_handler_unregister()?
> 
> This looks like it comes from the TX aggregation code added by the
> commit in the Fixes: tag.
> 
>>  			dev_kfree_skb_any(skb);
>>  			return;
>>  		}

Both comments from Sashiko seem valid, thanks.
I will make a v2 for the first one, and create a dedicated patch for
the second one.

Jérémy

pw-bot: cr