Re: [PATCH] net/rxrpc: Fix a use after free in rxrpc_input_packet
Lv Yunlong wrote: > In the case RXRPC_PACKET_TYPE_DATA of rxrpc_input_packet, if > skb_unshare(skb,..) failed, it will free the skb and return NULL. > But if skb_unshare() return NULL, the freed skb will be used by > rxrpc_eaten_skb(skb,..). That's not precisely the case: void rxrpc_eaten_skb(struct sk_buff *skb, enum rxrpc_skb_trace op) { const void *here = __builtin_return_address(0); int n = atomic_inc_return(_n_rx_skbs); trace_rxrpc_skb(skb, op, 0, n, 0, here); } The only thing that happens to skb here is that it's passed to trace_rxrpc_skb(), but that doesn't dereference it either. The *address* is used for display purposes, but that's all. > I see that rxrpc_eaten_skb() is used to drop a ref of skb. It isn't. > As the skb is already freed in skb_unshare() on error, my patch removes the > rxrpc_eaten_skb() to avoid the uaf. But you remove the accounting, which might lead to an assertion failure in af_rxrpc_exit(). That said, rxrpc_eaten_skb() should probably decrement rxrpc_n_rx_skbs, not increment it... David
[PATCH] net/rxrpc: Fix a use after free in rxrpc_input_packet
In the case RXRPC_PACKET_TYPE_DATA of rxrpc_input_packet, if skb_unshare(skb,..) failed, it will free the skb and return NULL. But if skb_unshare() return NULL, the freed skb will be used by rxrpc_eaten_skb(skb,..). I see that rxrpc_eaten_skb() is used to drop a ref of skb. As the skb is already freed in skb_unshare() on error, my patch removes the rxrpc_eaten_skb() to avoid the uaf. Fixes: d0d5c0cd1e711 ("rxrpc: Use skb_unshare() rather than skb_cow_data()") Signed-off-by: Lv Yunlong --- net/rxrpc/input.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c index dc201363f2c4..9ba408064465 100644 --- a/net/rxrpc/input.c +++ b/net/rxrpc/input.c @@ -1281,10 +1281,8 @@ int rxrpc_input_packet(struct sock *udp_sk, struct sk_buff *skb) */ if (sp->hdr.securityIndex != 0) { struct sk_buff *nskb = skb_unshare(skb, GFP_ATOMIC); - if (!nskb) { - rxrpc_eaten_skb(skb, rxrpc_skb_unshared_nomem); + if (!nskb) goto out; - } if (nskb != skb) { rxrpc_eaten_skb(skb, rxrpc_skb_received); -- 2.25.1