dccp: check for ccid in ccid_hc_tx_send_packet

[PATCH net] dccp: check for ccid in ccid_hc_tx_send_packet

Posted by Bragatheswaran Manickavel 2 years, 1 month ago

ccid_hc_tx_send_packet might be called with a NULL ccid pointer
leading to a NULL pointer dereference

Below mentioned commit has similarly changes
commit 276bdb82dedb ("dccp: check ccid before dereferencing")

Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c71bc336c5061153b502
Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
---
 net/dccp/ccid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dccp/ccid.h b/net/dccp/ccid.h
index 105f3734dadb..1015dc2b9392 100644
--- a/net/dccp/ccid.h
+++ b/net/dccp/ccid.h
@@ -163,7 +163,7 @@ static inline int ccid_packet_dequeue_eval(const int return_code)
 static inline int ccid_hc_tx_send_packet(struct ccid *ccid, struct sock *sk,
 					 struct sk_buff *skb)
 {
-	if (ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
+	if (ccid != NULL && ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
 		return ccid->ccid_ops->ccid_hc_tx_send_packet(sk, skb);
 	return CCID_PACKET_SEND_AT_ONCE;
 }
-- 
2.34.1

Re: [PATCH net] dccp: check for ccid in ccid_hc_tx_send_packet

Posted by Paolo Abeni 2 years, 1 month ago

On Sat, 2023-10-28 at 20:11 +0530, Bragatheswaran Manickavel wrote:
> ccid_hc_tx_send_packet might be called with a NULL ccid pointer
> leading to a NULL pointer dereference

You should describe how such event could happen.

> Below mentioned commit has similarly changes
> commit 276bdb82dedb ("dccp: check ccid before dereferencing")
> 
> Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c71bc336c5061153b502

and add a suitable fixes here.

(beyond taking care of other critical code paths, as reported by Eric).

Thanks!

Paolo

Re: [PATCH net] dccp: check for ccid in ccid_hc_tx_send_packet

Posted by Eric Dumazet 2 years, 1 month ago

On Sat, Oct 28, 2023 at 4:41 PM Bragatheswaran Manickavel
<bragathemanick0908@gmail.com> wrote:
>
> ccid_hc_tx_send_packet might be called with a NULL ccid pointer
> leading to a NULL pointer dereference
>
> Below mentioned commit has similarly changes
> commit 276bdb82dedb ("dccp: check ccid before dereferencing")
>
> Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c71bc336c5061153b502
> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
> ---
>  net/dccp/ccid.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/dccp/ccid.h b/net/dccp/ccid.h
> index 105f3734dadb..1015dc2b9392 100644
> --- a/net/dccp/ccid.h
> +++ b/net/dccp/ccid.h
> @@ -163,7 +163,7 @@ static inline int ccid_packet_dequeue_eval(const int return_code)
>  static inline int ccid_hc_tx_send_packet(struct ccid *ccid, struct sock *sk,
>                                          struct sk_buff *skb)
>  {
> -       if (ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
> +       if (ccid != NULL && ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
>                 return ccid->ccid_ops->ccid_hc_tx_send_packet(sk, skb);
>         return CCID_PACKET_SEND_AT_ONCE;
>  }
> --
> 2.34.1
>

If you are willing to fix dccp, I would make sure that some of
lockless accesses to dccps_hc_tx_ccid
are also double checked and fixed.

do_dccp_getsockopt() and dccp_get_info()

Re: [PATCH net] dccp: check for ccid in ccid_hc_tx_send_packet

Posted by Bragatheswaran Manickavel 2 years, 1 month ago

On 30/10/23 14:29, Eric Dumazet wrote:
> On Sat, Oct 28, 2023 at 4:41 PM Bragatheswaran Manickavel
> <bragathemanick0908@gmail.com> wrote:
>> ccid_hc_tx_send_packet might be called with a NULL ccid pointer
>> leading to a NULL pointer dereference
>>
>> Below mentioned commit has similarly changes
>> commit 276bdb82dedb ("dccp: check ccid before dereferencing")
>>
>> Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=c71bc336c5061153b502
>> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
>> ---
>>   net/dccp/ccid.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/dccp/ccid.h b/net/dccp/ccid.h
>> index 105f3734dadb..1015dc2b9392 100644
>> --- a/net/dccp/ccid.h
>> +++ b/net/dccp/ccid.h
>> @@ -163,7 +163,7 @@ static inline int ccid_packet_dequeue_eval(const int return_code)
>>   static inline int ccid_hc_tx_send_packet(struct ccid *ccid, struct sock *sk,
>>                                           struct sk_buff *skb)
>>   {
>> -       if (ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
>> +       if (ccid != NULL && ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
>>                  return ccid->ccid_ops->ccid_hc_tx_send_packet(sk, skb);
>>          return CCID_PACKET_SEND_AT_ONCE;
>>   }
>> --
>> 2.34.1
>>
> If you are willing to fix dccp, I would make sure that some of
> lockless accesses to dccps_hc_tx_ccid
> are also double checked and fixed.
>
> do_dccp_getsockopt() and dccp_get_info()


Hi Eric,

In both do_dccp_getsockopt() and dccp_get_info(), dccps_hc_rx_ccid are 
checked properly before access.

Thanks,
Bragathe

Re: [PATCH net] dccp: check for ccid in ccid_hc_tx_send_packet

Posted by Eric Dumazet 2 years, 1 month ago

On Mon, Oct 30, 2023 at 4:40 PM Bragatheswaran Manickavel
<bragathemanick0908@gmail.com> wrote:
>
>
> On 30/10/23 14:29, Eric Dumazet wrote:
> > On Sat, Oct 28, 2023 at 4:41 PM Bragatheswaran Manickavel
> > <bragathemanick0908@gmail.com> wrote:
> >> ccid_hc_tx_send_packet might be called with a NULL ccid pointer
> >> leading to a NULL pointer dereference
> >>
> >> Below mentioned commit has similarly changes
> >> commit 276bdb82dedb ("dccp: check ccid before dereferencing")
> >>
> >> Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
> >> Closes: https://syzkaller.appspot.com/bug?extid=c71bc336c5061153b502
> >> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
> >> ---
> >>   net/dccp/ccid.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/dccp/ccid.h b/net/dccp/ccid.h
> >> index 105f3734dadb..1015dc2b9392 100644
> >> --- a/net/dccp/ccid.h
> >> +++ b/net/dccp/ccid.h
> >> @@ -163,7 +163,7 @@ static inline int ccid_packet_dequeue_eval(const int return_code)
> >>   static inline int ccid_hc_tx_send_packet(struct ccid *ccid, struct sock *sk,
> >>                                           struct sk_buff *skb)
> >>   {
> >> -       if (ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
> >> +       if (ccid != NULL && ccid->ccid_ops->ccid_hc_tx_send_packet != NULL)
> >>                  return ccid->ccid_ops->ccid_hc_tx_send_packet(sk, skb);
> >>          return CCID_PACKET_SEND_AT_ONCE;
> >>   }
> >> --
> >> 2.34.1
> >>
> > If you are willing to fix dccp, I would make sure that some of
> > lockless accesses to dccps_hc_tx_ccid
> > are also double checked and fixed.
> >
> > do_dccp_getsockopt() and dccp_get_info()
>
>
> Hi Eric,
>
> In both do_dccp_getsockopt() and dccp_get_info(), dccps_hc_rx_ccid are
> checked properly before access.
>

Not really, because another thread can change the value at the same time.

Adding checks is not solving races.