> On Wed, Jan 03, 2018 at 04:06:28PM +0100, Lorenzo Bianconi wrote:
>> I agree to remove offset parameter in this case. What about (as
>> already suggested by James) to take into account possible alignment
>> issues with previous version of L2TPv3 protocol using 'L2 specific
>> sublayer'?
>>
> I think James was refering to the general architecture of L2TPv3, where
> such issues should be handled by pseudo-wire specific headers. I don't
> think he was talking about implementing arbitrary padding using an
> L2 specific sublayer. None of the standardised headers allow arbitrary
> padding. And implementing our own would make us imcompatible with any
> other implementation.
>
>> I guess, on the kernel side (we will need to patch iproute2 on
>> userspace side), we need just to properly initialized the 'l2specific'
>> field to 0 since otherwise we will have the same memleak issue there
>> if assume we can have l2specific_len != {0,4}.
>>
> That would produce the same frame format as what the 'offset' option
> was supposed to produce (if it did properly initialise its padding
> bits). That is, we'd have an arbitrary number of padding bits inserted
> between the l2-specific header and the l2 frame (L2TP's payload). These
> frames are invalid, and doing that brings us nothing compared to
> keeping the offset option.
>
>> Moreover does it worth to add some sanity checks in netlink code to
>> enforce the relation between l2specific_len and l2specific_type?
>>
> Yes, certainly.
>
>> At
>> the moment there are no guarantee that if l2specific_type is set to
>> L2TP_L2SPECTYPE_DEFAULT, l2specific_len will be grater or equal than
>> 4.
>>
> Thanks for pointing this out. That needs to be fixed. We don't support
> anything but the default l2spec layer (or no l2spec layer at all). So
> we should only accept L2TP_L2SPECTYPE_NONE and L2TP_L2SPECTYPE_DEFAULT.
>
> And we shouldn't need to use l2specific_len at all. The default l2spec
> header is always four bytes long, so we don't need to rely on a user
> supplied value. Looking at the code, it seems that invalid usage of
> L2TP_ATTR_L2SPEC_LEN allows leaking memory on the network or sending
> corrupted frames (depending on if its value is too small or too big).
>
> Do you want to take care of this?Ack, I am working on it. Regards, Lorenzo
