On Tue, Sep 15, 2015 at 05:22:40AM -0400, Gene Cumm via Syslinux wrote:> On Sep 10, 2015 1:32 AM, "celelibi--- via Syslinux" <syslinux at zytor.com> wrote: > > > > From: Sylvain Gault <sylvain.gault at gmail.com> > > > > Despite having native network capabilities, UEFI 2.4 (the most > > widely deployed at the moment) has no native DNS resolver. I propose > > here an implementation more or less inspired by the one found in > > core/legacynet/dnsresolv.c. > > > > Since it's non-trivial, I'd like to ask for a deep review of this > > code. I tried to make it as strong as possible against malformed > > packets and should be devoided of remote code execution or infinite > > loops (contrary to the legacynet implementation), but I may have > > forgotten something. I would take any comment, > > even on the coding style. :) > > > > I made a few tests, it worked correctly with a normal dns > > server. Didn't test malformed packets. > > > > Here are the two main missing features of this patch. > > - No caching implemented. > > - No timeout management. The efi implementation of core_udp_recv > > wait 15ms before giving up. > > > > I'd also like to ask if it wouldn't be better to just write an > > efi driver for lwip and just inherit all the network protocols > > implemented there, instead of trying to use the buggy native UEFI > > implementations. Depending on this, I may or may not spend time on > > this DNS resolver. > > > > Sylvain Gault (1): > > efi: Implemented a DNS stub resolver for efi > > > > efi/dns.c | 529 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > efi/pxe.c | 16 -- 2 files changed, 529 insertions(+), 16 > > deletions(-) create mode 100644 efi/dns.c > > Extending like this to reestablish equality between architectures > definitely is a good idea. >How to read 'a good idea' ? Does it mean "patch accepted"??? Groeten Geert Stappers -- Leven en laten leven
2015-09-25 21:27 UTC+02:00, Geert Stappers via Syslinux <syslinux at zytor.com>:> On Tue, Sep 15, 2015 at 05:22:40AM -0400, Gene Cumm via Syslinux wrote: >> On Sep 10, 2015 1:32 AM, "celelibi--- via Syslinux" <syslinux at zytor.com> >> wrote: >> > >> > From: Sylvain Gault <sylvain.gault at gmail.com> >> > >> > Despite having native network capabilities, UEFI 2.4 (the most >> > widely deployed at the moment) has no native DNS resolver. I propose >> > here an implementation more or less inspired by the one found in >> > core/legacynet/dnsresolv.c. >> > >> > Since it's non-trivial, I'd like to ask for a deep review of this >> > code. I tried to make it as strong as possible against malformed >> > packets and should be devoided of remote code execution or infinite >> > loops (contrary to the legacynet implementation), but I may have >> > forgotten something. I would take any comment, >> > even on the coding style. :) >> > >> > I made a few tests, it worked correctly with a normal dns >> > server. Didn't test malformed packets. >> > >> > Here are the two main missing features of this patch. >> > - No caching implemented. >> > - No timeout management. The efi implementation of core_udp_recv >> > wait 15ms before giving up. >> > >> > I'd also like to ask if it wouldn't be better to just write an >> > efi driver for lwip and just inherit all the network protocols >> > implemented there, instead of trying to use the buggy native UEFI >> > implementations. Depending on this, I may or may not spend time on >> > this DNS resolver. >> > >> > Sylvain Gault (1): >> > efi: Implemented a DNS stub resolver for efi >> > >> > efi/dns.c | 529 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> > efi/pxe.c | 16 -- 2 files changed, 529 insertions(+), 16 >> > deletions(-) create mode 100644 efi/dns.c >> >> Extending like this to reestablish equality between architectures >> definitely is a good idea. >> > > How to read 'a good idea' ? Does it mean "patch accepted"??? >In any case, I may submit another version (or additional patches if this one get merged) with a few improvements, like removing the recursion limit and retry the same servers several times as the UDP packets may get lost. And coding style improvement. Celelibi
On Fri, Sep 25, 2015 at 6:40 PM, Celelibi via Syslinux <syslinux at zytor.com> wrote:> 2015-09-25 21:27 UTC+02:00, Geert Stappers via Syslinux <syslinux at zytor.com>: >> On Tue, Sep 15, 2015 at 05:22:40AM -0400, Gene Cumm via Syslinux wrote: >>> On Sep 10, 2015 1:32 AM, "celelibi--- via Syslinux" <syslinux at zytor.com> >>> wrote: >>> > >>> > From: Sylvain Gault <sylvain.gault at gmail.com> >>> > >>> > Despite having native network capabilities, UEFI 2.4 (the most >>> > widely deployed at the moment) has no native DNS resolver. I propose >>> > here an implementation more or less inspired by the one found in >>> > core/legacynet/dnsresolv.c. >>> > >>> > Since it's non-trivial, I'd like to ask for a deep review of this >>> > code. I tried to make it as strong as possible against malformed >>> > packets and should be devoided of remote code execution or infinite >>> > loops (contrary to the legacynet implementation), but I may have >>> > forgotten something. I would take any comment, >>> > even on the coding style. :) >>> > >>> > I made a few tests, it worked correctly with a normal dns >>> > server. Didn't test malformed packets. >>> > >>> > Here are the two main missing features of this patch. >>> > - No caching implemented. >>> > - No timeout management. The efi implementation of core_udp_recv >>> > wait 15ms before giving up. >>> > >>> > I'd also like to ask if it wouldn't be better to just write an >>> > efi driver for lwip and just inherit all the network protocols >>> > implemented there, instead of trying to use the buggy native UEFI >>> > implementations. Depending on this, I may or may not spend time on >>> > this DNS resolver. >>> > >>> > Sylvain Gault (1): >>> > efi: Implemented a DNS stub resolver for efi >>> > >>> > efi/dns.c | 529 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> > efi/pxe.c | 16 -- 2 files changed, 529 insertions(+), 16 >>> > deletions(-) create mode 100644 efi/dns.c >>> >>> Extending like this to reestablish equality between architectures >>> definitely is a good idea. >>> >> >> How to read 'a good idea' ? Does it mean "patch accepted"??? >> > > In any case, I may submit another version (or additional patches if > this one get merged) with a few improvements, like removing the > recursion limit and retry the same servers several times as the UDP > packets may get lost. And coding style improvement.As you noted, you don't accommodate timeouts which is probably the most critical operation this doesn't have. We can't stay in the query forever. jiffies() and ms_timer() are the two available in the core. core/legacynet/dnsresolv.c used jiffies() but ms_timer() should be dealing with millisecond values, not 55ms jiffies and about CLK_TCK jiffies per second. An alternative way to just stub it is to merge everything but the actual call that starts sending UDP packets or just the most basic functionality you've implemented first. -- -Gene