Re: [PATCH net] can: raw: fix ro->uniq use-after-free in raw_rcv()

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 08.04.26 19:22, Sam P wrote:
> On 08/04/2026 17:28, Oliver Hartkopp wrote:

>>
>> Can you tell why you preferred the destructor solution now?
> 
> Thank you :) I preferred the destructor solution as it seemed to match 
> the socket lifetime model better and I wasn't sure if the blocking sync 
> in the raw_release() was too heavy-handed for this specific issue, given 
> raw_release() already holds rtnl_lock() and lock_sock(sk). That said, 
> I'm happy to defer to your experience if the sync fix is better suited, 
> I have tested both of them.

Thanks. I think rtnl_lock() really might create a performance impact to 
other networking code when syncronize_rcu() waits for its grace period.

>> And if I see it correctly the UAF problem might also show up with the
>> kfree(ro->filter) statement we can see at the beginning of the above 
>> patch.
>>
>> So either free_percpu(ro->uniq) and kfree(ro->filter) should be 
>> handled after the finalized synchronize_rcu() process, right?
> 
> ro->filter isn't accessed in the racey raw_rcv() path as far as I can 
> tell, and I don't *think* there are other racey paths but it wouldn't 
> hurt to handle it just in-case. I think this would be simple with the 
> synchronize_rcu() patch, as you mentioned, but I'm not sure with the 
> destructor.

ro->filter contains all the CAN_RAW specific CAN ID filters and is 
allocated if more than the single default filter is required.

It is last used in the raw_disable_allfilters() above.

So after the good discussion I tend to your original approach with the 
destructor ;-)

Will add my Acked-by: to the original posted patch.

Many thanks,
Oliver