netdev - Re: [PATCH next] neigh: initialize neigh entry correctly during arp processing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Aug 2017 18:02:33 -0700
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     Mahesh Bandewar <mahesh@...dewar.net>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        netdev <netdev@...r.kernel.org>,
        Ido Schimmel <idosch@...lanox.com>,
        Hans Liljestrand <ishkamiel@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Reshetova Elena <elena.reshetova@...el.com>,
        Florian Westphal <fw@...len.de>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        Ihar Hrachyshka <ihrachys@...hat.com>,
        David Ahern <dsa@...ulusnetworks.com>,
        Zhang Shengju <zhangshengju@...s.chinamobile.com>
Subject: Re: [PATCH next] neigh: initialize neigh entry correctly during arp processing

On Wed, Aug 16, 2017 at 5:32 PM, Sowmini Varadhan
<sowmini.varadhan@...cle.com> wrote:
> On (08/16/17 17:02), Mahesh Bandewar wrote:
>>
>> From: Mahesh Bandewar <maheshb@...gle.com>
>>
>> If the ARP processing creates a neigh entry, it's immediately marked
>> as STALE without timer and stays that way in that state as long as
>> host do not send traffic to that neighbour.
>
> Perhaps I dont understand the specific  packet exchange case
> that your patch is trying to fix,  but if the neighbor entry
> is created as a result of an incoming packet (but we have not
> yet sent anything to this neighbor) then it should be marked STALE?
> IOW, STALE means "Ingress path claims this adjacency, but egress
> path has not been verified".  Is the problem that the neigh never
> goes into PROBE?
>
Correct. The entry gets created  (NUD_NONE) and few jiffies later it
gets marked as STALE. It's doesn't even get a chance to get PROBEd.

>> +     if (neigh) {
>> +             if (neigh->nud_state & NUD_VALID)
>> +                     neigh_update(neigh, lladdr, NUD_STALE,
>> +                                  NEIGH_UPDATE_F_OVERRIDE, 0);
>> +             else
>> +                     neigh_event_send(neigh, NULL);
>> +     }
>
> NUD_VALID is already a mask of
>  (NUD_PERMANENT|NUD_NOARP|NUD_REACHABLE|NUD_PROBE|NUD_STALE|NUD_DELAY)
> are you sure you want to force the transition of probe/delay -> stale
> here?
It's not forced and neigh_update() has guards / provision to consider
these type of transition and wont force it. Just that if the state is
not-valid (NUD_INCOMPLETE | NUD_NONE) etc then it marks it STALE and
deletes the timer.

> Maybe it woudl help to describe the exact wire packet
> exchange that is broken today, but fixed by your patch.
>
> --Sowmini
>
>