Added PXE DHCP options #559
Conversation
Qubasa
force-pushed
the
pxe_support
branch
3 times, most recently
from
e41c8ec
to
7064337
Compare
There was a problem hiding this comment.
Thanks for the PR! Code looks good.
The DHCP fuzzer (#562) found a few issues though.
- Out of bounds at line 215 if option data length is 0.
- emit panics at line 306, maybe something wrong in
buffer_len.
Could you fix them, and then run the fuzzer yourself to check there are no more? If you get the fuzzer to run for 10-15min without finding anything it should be good to go.
| @@ -166,13 +362,39 @@ impl<'a> DhcpOption<'a> { | |||
| } | |||
| _ => { | |||
| skip_length = self.buffer_len(); | |||
|
|
|||
| assert!(skip_length <= buffer.len()); | |||
There was a problem hiding this comment.
@whitequark @Dirbaio I have a problem I can't return an error in this function, so in the case someone puts an 'Other' option with a very large payload into the function and the supplied buffer is smaller, then this function panics. I don't want to silently return and ignore the option either. But the fuzzer will find this crash and stop there. What I did now is insert an assert so the panic is easier to reason about
There was a problem hiding this comment.
It's OK for emit to panic if the supplied buffer len is not exactly equal to the length provided by buffer_len.It's the caller's responsibility to ensure that.
I guess it's either a bug in the fuzzer (but from a quick look it looks OK to me) or a mismatch in the length you return from buffer_len and the length you require in emit...
There was a problem hiding this comment.
@Dirbaio Ah alright yeah the Repr returns the incorrect size and the fuzzer allocates exactly that size, I see! As I'm looking into it that bug already existed beforehand
|
If someone wants to open up a new reworked pull request go ahead :-) I sadly won't be able to work on this for at least a couple of months sadly |
I added three new DHCP options for PXE boot defined in this RFC4578
Appropriate tests have been added too. I also added two option fields vendor id and time offset from RFC2132