Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data

[Michal Luczaj <mhal@rbox.co>]

On 12/19/24 15:48, Stefano Garzarella wrote:
> On Thu, 19 Dec 2024 at 15:36, Michal Luczaj <mhal@rbox.co> wrote:
>>
>> On 12/19/24 09:19, Stefano Garzarella wrote:
>>> ...
>>> I think the best thing though is to better understand how to handle
>>> deassign, rather than checking everywhere that it's not null, also
>>> because in some cases (like the one in virtio-vsock), it's also
>>> important that the transport is the same.
>>
>> My vote would be to apply your virtio_transport_recv_pkt() patch *and* make
>> it impossible-by-design to switch ->transport from non-NULL to NULL in
>> vsock_assign_transport().
> 
> I don't know if that's enough, in this case the problem is that some
> response packets are intended for a socket, where the transport has
> changed. So whether it's null or assigned but different, it's still a
> problem we have to handle.
> 
> So making it impossible for the transport to be null, but allowing it
> to be different (we can't prevent it from changing), doesn't solve the
> problem for us, it only shifts it.

Got it. I assumed this issue would be solved by `vsk->transport !=
&t->transport` in the critical place(s).

(Note that BPF doesn't care if transport has changed; BPF just expects to
have _a_ transport.)

>> If I'm not mistaken, that would require rewriting vsock_assign_transport()
>> so that a new transport is assigned only once fully initialized, otherwise
>> keep the old one (still unhurt and functional) and return error. Because
>> failing connect() should not change anything under the hood, right?
>>
> 
> Nope, connect should be able to change the transport.
> 
> Because a user can do an initial connect() that requires a specific
> transport, this one fails maybe because there's no peer with that cid.
> Then the user can redo the connect() to a different cid that requires
> a different transport.

But the initial connect() failing does not change anything under the hood
(transport should/could stay NULL). Then a successful re-connect assigns
the transport (NULL -> non-NULL). And it's all good because all I wanted to
avoid (because of BPF) was non-NULL -> NULL. Anyway, that's my possibly
shallow understanding :)