Date:   Mon, 10 Jun 2019 23:45:39 -0700
From:   Zhongjie Wang <zwang048@....edu>
To:     Neal Cardwell <ncardwell@...gle.com>
Cc:     Netdev <netdev@...r.kernel.org>, Zhiyun Qian <zhiyunq@...ucr.edu>
Subject: Re: tp->copied_seq used before assignment in tcp_check_urg

Hi Neal,

Thanks for your valuable feedback! Yes, I think you are right.
It seems not a problem if tp->urg_data and tp->urg_seq are used together.
>From our test results, we can only see there are some paths requiring
specific initial sequence number to reach.
But as you said, it would not cause a difference in the code logic.
We haven't observed any abnormal states.

Thanks,
Zhongjie Wang
Ph.D. Candidate 2015 Fall
Department of Computer Science & Engineering
University of California, Riverside


On Mon, Jun 10, 2019 at 7:19 PM Neal Cardwell <ncardwell@...gle.com> wrote:
>
> On Mon, Jun 10, 2019 at 7:48 PM Zhongjie Wang <zwang048@....edu> wrote:
> >
> > Hi Neal,
> >
> > Thanks for your reply. Sorry, I made a mistake in my previous email.
> > After I double checked the source code, I think it should be tp->urg_seq,
> > which is used before assignment, instead of tp->copied_seq.
> > Still in the same if-statement:
> >
> > 5189     if (tp->urg_seq == tp->copied_seq && tp->urg_data &&
> > 5190         !sock_flag(sk, SOCK_URGINLINE) && tp->copied_seq != tp->rcv_nxt) {
> > 5191         struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
> > 5192         tp->copied_seq++;
> > 5193         if (skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq)) {
> > 5194             __skb_unlink(skb, &sk->sk_receive_queue);
> > 5195             __kfree_skb(skb);   // wzj(a)
> > 5196         }
> > 5197     }
> > 5198
> > 5199     tp->urg_data = TCP_URG_NOTYET;
> > 5200     tp->urg_seq = ptr;
> >
> > It compares tp->copied_seq with tp->urg_seq.
> > And I found only 1 assignment of tp->urg_seq in the code base,
> > which is after the if-statement in the same tcp_check_urg() function.
> >
> > So it seems tp->urg_seq is not assigned to any sequence number before
> > its first use.
> > Is that correct?
>
> I agree, it does seem that tp->urg_seq is not assigned to any sequence
> number before its first use.
>
> AFAICT from a quick read of the code, this does not matter. It seems
> the idea is for tp->urg_data and tp->urg_seq to be set and used
> together, so that tp->urg_seq is never relied upon to be set to
> something meaningful unless tp->urg_data has also been verified to be
> set to something (something non-zero).
>
> I suppose it might be more clear to structure the code to check urg_data first:
>
>   if (tp->urg_data && tp->urg_seq == tp->copied_seq &&
>
> ...but in practice AFAICT it does not make a difference, since no
> matter which order the expressions use, both conditions must be true
> for the code to have any side effects.
>
> > P.S. In our symbolic execution tool, we found an execution path that
> > requires the client initial sequence number (ISN) to be 0xFF FF FF FF.
> > And when it traverse that path, the tp->copied_seq is equal to (client
> > ISN + 1), and compared with 0 in this if-statatement.
> > Therefore the client ISN has to be exactly 0xFF FF FF FF to hit this
> > execution path.
> >
> > To trigger this, we first sent a SYN packet, and then an ACK packet
> > with urgent pointer.
>
> Does your test show any invalid behavior by the TCP endpoint? For
> example, does the state in tcp_sock become incorrect, or is some
> system call return value or outgoing packet incorrect? AFAICT from the
> scenario you describe it seems that the "if" condition would fail when
> the receiver processes the ACK packet with urgent pointer, because
> tp->urg_data was not yet set at this point. Thus it would seem that in
> this case it does not matter that tp->urg_seq is not assigned to any
> sequence number before being first used.
>
> cheers,
> neal

Powered by blists - more mailing lists