Re: Kernel memory corruption in CIPSO labeled TCP packets processing.

[Paul Moore <paul@paul-moore.com>]

On Fri, Jan 18, 2019 at 11:34 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> Hi, Paul!
> I don't like this. As you said, fix any calls to icmp_send is a trivial.
> But in cipso_v4_optptr, we repeating the work already done, and in cipso_v4_error
> we will need to do even more again.

Yes, this is extra overhead, but it is in an error path so I'm not
overly concerned about the performance impact on the given connection.

I will admit that it isn't an ideal solution from a LSM/NetLabel
perspective, but historically the netdev folks have not been overly
receptive to changes which negatively impact the core network stack
regardless of how important it may be for the LSMs.  If we could
modify icmp_send() so that it works regardless of the caller's
location in the stack, that would be great, I'm just not sure it would
be deemed acceptable.  I suggested the approach I did because I think
that has the best chance of getting merged.

Regardless, I would encourage you to put together a patch and post it
for review, I think that's the best way to get a response.  If you
aren't able, or aren't comfortable, submitting a patch please let me
know.

> Any code, working with IP header data on several
> levels of TCP/IP stack need to do this again and again. Is looks too overloaded!
> In my opinion, this is a problem of TCP/IP stack design, comes from standing
> compiled IP header data in different places at different stack layers.
> May be better to add some flag or pointer to struct sk_buff?
> But, I think, we need netdev developers advice for this.
> Regards, Sergey.
>
> 18.01.2019, 17:53, "Paul Moore" <paul@paul-moore.com>:
> > On Tue, Jan 15, 2019 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > It's been a few days now with no comments from the netdev folks, so I
> > think it's probably best to start putting together a RFC patch for
> > review/comment. Nazarov, would you like to do that? If not, that's
> > okay, just let me know.
> >
> > I'm still concerned about calling ip_options_compile() in icmp_send()
> > and I'm thinking we might be better off to add a new ip_options
> > parameter to icmp_send(); if the parameter is NULL we behave as we do
> > today, but if it is non-NULL we use the given ip_options parameter in
> > place of calling ip_options_echo(). With that change in place, we
> > would need to update cipso_v4_error() to do the extra work of calling
> > ip_options_compile() and __ip_options_echo(). There looks to be maybe
> > a dozen (or two?) existing icmp_send() callers, but it should be
> > pretty trivial to update them to pass NULL for the new parameter.
> >
> > What do you think?

-- 
paul moore
www.paul-moore.com