Re: [PATCH nf] netfilter: nf_conntrack_sip: fix use of uninitialized rtp_addr in process_sdp

[Pablo Neira Ayuso]

On Thu, Mar 26, 2026 at 09:44:17PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > @@ -1091,9 +1095,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
> > >  					  &matchoff, &matchlen, &maddr) > 0) {
> > >  			maddr_len = matchlen;
> > >  			memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
> > > -		} else if (caddr_len)
> > > +			have_rtp_addr = true;
> > > +		} else if (caddr_len) {
> > >  			memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> > > -		else {
> > > +			have_rtp_addr = true;
> > 
> > After this update, this loop sets over rtp_addr, but this was already
> > set by ct_sip_parse_sdp_addr() a bit above.
> > 
> > This new chunk results in:
> > 
> >                 } else if (caddr_len) {
> >                        memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> >                        have_rtp_addr = true;
> > 
> > which is not needed? Why does caddr need to be copied over and over
> > again to rtp_addr?
> 
> Code before update was:
>                 if (ct_sip_parse_sdp_addr(ct, *dptr, mediaoff, *datalen,
>                                           SDP_HDR_CONNECTION, SDP_HDR_MEDIA,
>                                           &matchoff, &matchlen, &maddr) > 0) {
>                         maddr_len = matchlen;
>                         memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
>                 } else if (caddr_len)
>                         memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
>                 else {
> 
> 
> so we re-set rtp_addr to the session description in case it was
> overwritten by earlier iteration of the loop.
> 
> 1. Extract session description (caddr_len set)
> 2. enter loop, parse media description (overwrite rtp_addr with media
>    address)
> 3. next loop fails ct_sip_parse_sdp_addr() call
>    Restore the original session address instead of using
>    the previous media description.
> 
> I think its correct this way.

Thanks for explaining, this is leaving things as they were before this
patch.