[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.