On Mon, Jun 10, 2019 at 7:48 PM Zhongjie Wang <zwang...@ucr.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
