Skip to content

lldpd:Device hangs after running LLDP suite in Defensics fuzz tool#420

Closed
sudhanshukumar22 wants to merge 1 commit intolldpd:masterfrom
sudhanshukumar22:lldp_median_length_issue
Closed

lldpd:Device hangs after running LLDP suite in Defensics fuzz tool#420
sudhanshukumar22 wants to merge 1 commit intolldpd:masterfrom
sudhanshukumar22:lldp_median_length_issue

Conversation

@sudhanshukumar22
Copy link
Copy Markdown

Running Defensics LLDP suite often hangs the device and only way to retrieve it back is by power cycling the device.

As the device hangs and there is no access to Console/Mgmt Sessions, not able to take tech-support output in problem state.

Attaching the pcap files of LLDP packets from Defensics server, as the report doesn't contain any failures.

Also attaching the syslog output before/at the time hitting problem state.

Additionally, I checked if there are any sporadic CPU/Memory hike while running and did not notice any.

Only once seen lldp crash and attaching the same as well.
Fix: added a validation check in MED processing in lldp_decode().
Signed-off-by: sudhanshukumar22 sudhanshu.kumar@broadcom.com

…etrieve it back is by power cycling the device.

As the device hangs and there is no access to Console/Mgmt Sessions, not able to take tech-support output in problem state.

Attaching the pcap files of LLDP packets from Defensics server, as the  report doesn't contain any failures.

Also attaching the syslog output before/at the time hitting problem state.

Additionally, I checked if there are any sporadic CPU/Memory hike while running and did not notice any.

Only once seen lldp crash and attaching the same as well.
Fix: added a validation check in MED processing in lldp_decode().
Signed-off-by: sudhanshukumar22 <sudhanshu.kumar@broadcom.com>
@vincentbernat
Copy link
Copy Markdown
Member

Thanks for your patch. However, it only works when the location is a civic address. The two other possibilities (coordinate and ELIN) does use the first byte as a length. In lldp.c, no decoding is done, so an incorrect value shouldn't matter. Also, looking in atom/med.c, I see the length is just ignored (we rely on the len of the TLV, which is not totally correct). Do you have a PCAP showing a packet exhibiting a problem?

@sudhanshukumar22
Copy link
Copy Markdown
Author

sudhanshukumar22 commented Dec 3, 2020

Thanks for your patch. However, it only works when the location is a civic address. The two other possibilities (coordinate and ELIN) does use the first byte as a length. In lldp.c, no decoding is done, so an incorrect value shouldn't matter. Also, looking in atom/med.c, I see the length is just ignored (we rely on the len of the TLV, which is not totally correct). Do you have a PCAP showing a packet exhibiting a problem?

I am attaching the lldp packet capture: Please ignore the name.
mgmt ip change issue.zip

@vincentbernat
Copy link
Copy Markdown
Member

Thanks, I am running into a bit of trouble to run some tests. Hopefully, this will be solved next week.

vincentbernat added a commit that referenced this pull request Dec 6, 2020
Some bounds were not checked correctly when parsing LLDP-MED civic
location fields. This triggers out-of-bound reads (no write) in
lldpcli, ultimately leading to a crash.

Fix #420
@vincentbernat
Copy link
Copy Markdown
Member

If you can run again the test suite, it will be helpful to catch remaining bugs!

@sudhanshukumar22
Copy link
Copy Markdown
Author

@vincentbernat : Does your pull request 422 fix this issue ?

@vincentbernat
Copy link
Copy Markdown
Member

Yes, it should. I was able to reproduce your issue.

lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 5, 2021
The details are as follows:

    1. 0010-Ported-fix-for-length-exceeded-from-lldp-community.patch

    Patch taken from lldpd/lldpd@7824347

    lib: remove limit on system description length

    The limit was introduced in 9c49ced while fixing a memory leak.
    The state data is used to ensure we don't interleave operations. We
    need to handle the case where the value is truncated because it is
    larger than the allocated size.

    Fix issue lldpd/lldpd#408

    2. 0011-fix-med-location-len.patch
    
    Patch taken from lldpd/lldpd@5c34794

    lib: fix LLDP-MED location parsing in liblldpctl

    Some bounds were not checked correctly when parsing LLDP-MED civic
    location fields. This triggers out-of-bound reads (no write) in
    lldpcli, ultimately leading to a crash.

    Fix lldpd/lldpd#420

Signed-off-by: sudhanshukumar22 <sudhanshu.kumar@broadcom.com>
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 5, 2021
The details are as follows:

    1. 0010-Ported-fix-for-length-exceeded-from-lldp-community.patch

    Patch taken from lldpd/lldpd@7824347

    lib: remove limit on system description length

    The limit was introduced in 9c49ced while fixing a memory leak.
    The state data is used to ensure we don't interleave operations. We
    need to handle the case where the value is truncated because it is
    larger than the allocated size.

    Fix issue lldpd/lldpd#408

    2. 0011-fix-med-location-len.patch
    
    Patch taken from lldpd/lldpd@5c34794

    lib: fix LLDP-MED location parsing in liblldpctl

    Some bounds were not checked correctly when parsing LLDP-MED civic
    location fields. This triggers out-of-bound reads (no write) in
    lldpcli, ultimately leading to a crash.

    Fix lldpd/lldpd#420

Signed-off-by: sudhanshukumar22 <sudhanshu.kumar@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants