* [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb
@ 2017-05-18 16:01 Davide Caratti
2017-05-19 8:41 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Davide Caratti @ 2017-05-18 16:01 UTC (permalink / raw)
To: netfilter-devel; +Cc: pablo, Florian Westphal, Marcelo Ricardo Leitner
sctp_compute_cksum() implementation assumes that at least the SCTP header
is in the linear part of skb: modify conntrack error callback to avoid
false CRC32c mismatch, if the transport header is partially/entirely paged.
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/netfilter/nf_conntrack_proto_sctp.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 13875d5..1c5b14a 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -512,16 +512,19 @@ static int sctp_error(struct net *net, struct nf_conn *tpl, struct sk_buff *skb,
u8 pf, unsigned int hooknum)
{
const struct sctphdr *sh;
- struct sctphdr _sctph;
const char *logmsg;
- sh = skb_header_pointer(skb, dataoff, sizeof(_sctph), &_sctph);
- if (!sh) {
+ if (skb->len < dataoff + sizeof(struct sctphdr)) {
logmsg = "nf_ct_sctp: short packet ";
goto out_invalid;
}
if (net->ct.sysctl_checksum && hooknum == NF_INET_PRE_ROUTING &&
skb->ip_summed == CHECKSUM_NONE) {
+ if (!skb_make_writable(skb, dataoff + sizeof(struct sctphdr))) {
+ logmsg = "nf_ct_sctp: failed to read header ";
+ goto out_invalid;
+ }
+ sh = (const struct sctphdr *)(skb->data + dataoff);
if (sh->checksum != sctp_compute_cksum(skb, dataoff)) {
logmsg = "nf_ct_sctp: bad CRC ";
goto out_invalid;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb
2017-05-18 16:01 [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb Davide Caratti
@ 2017-05-19 8:41 ` Pablo Neira Ayuso
2017-05-19 11:39 ` Davide Caratti
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-05-19 8:41 UTC (permalink / raw)
To: Davide Caratti; +Cc: netfilter-devel, Florian Westphal, Marcelo Ricardo Leitner
Hi Davide,
On Thu, May 18, 2017 at 06:01:43PM +0200, Davide Caratti wrote:
> sctp_compute_cksum() implementation assumes that at least the SCTP header
> is in the linear part of skb: modify conntrack error callback to avoid
> false CRC32c mismatch, if the transport header is partially/entirely paged.
I guess you considered this, but I would like to know the reason for
this approach. Why not fix this from sctp_compute_cksum()? I mean, I
can see other spots in the kernel tree that may be affected by this?
Or is it that you're only observing this from a path that is specific
of conntrack?
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/netfilter/nf_conntrack_proto_sctp.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index 13875d5..1c5b14a 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -512,16 +512,19 @@ static int sctp_error(struct net *net, struct nf_conn *tpl, struct sk_buff *skb,
> u8 pf, unsigned int hooknum)
> {
> const struct sctphdr *sh;
> - struct sctphdr _sctph;
> const char *logmsg;
>
> - sh = skb_header_pointer(skb, dataoff, sizeof(_sctph), &_sctph);
> - if (!sh) {
> + if (skb->len < dataoff + sizeof(struct sctphdr)) {
> logmsg = "nf_ct_sctp: short packet ";
> goto out_invalid;
> }
> if (net->ct.sysctl_checksum && hooknum == NF_INET_PRE_ROUTING &&
> skb->ip_summed == CHECKSUM_NONE) {
> + if (!skb_make_writable(skb, dataoff + sizeof(struct sctphdr))) {
> + logmsg = "nf_ct_sctp: failed to read header ";
> + goto out_invalid;
> + }
> + sh = (const struct sctphdr *)(skb->data + dataoff);
> if (sh->checksum != sctp_compute_cksum(skb, dataoff)) {
> logmsg = "nf_ct_sctp: bad CRC ";
> goto out_invalid;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb
2017-05-19 8:41 ` Pablo Neira Ayuso
@ 2017-05-19 11:39 ` Davide Caratti
2017-05-23 13:51 ` Davide Caratti
0 siblings, 1 reply; 6+ messages in thread
From: Davide Caratti @ 2017-05-19 11:39 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netfilter-devel, Florian Westphal, Marcelo Ricardo Leitner
hello Pablo, thank you for looking at this!
On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote:
> On Thu, May 18, 2017 at 06:01:43PM +0200, Davide Caratti wrote:
> > sctp_compute_cksum() implementation assumes that at least the SCTP header
> > is in the linear part of skb: modify conntrack error callback to avoid
> > false CRC32c mismatch, if the transport header is partially/entirely paged.
>
> I guess you considered this, but I would like to know the reason for
> this approach. Why not fix this from sctp_compute_cksum()?
I think sctp_compute_cksum() is legitimately needing the transport header
i
n the linear data of skb, because it needs to set to zero 4 octects at
CRC32c offset before computing the CRC32c (as per RFC3309 §2.1). Since
these are the last 4 octects of the SCTP header, then we need to
__pskb_pull_tail() on the whole header, if some/all of its members are
paged.
> I mean, I can see other spots in the kernel tree that may be affected by this?
> Or is it that you're only observing this from a path that is specific
> of conntrack?
I did the check before posting, and the kernel code seemed to already
ensure skb is writable until SCTP header + sizeof(SCTP header) offset,
before calling sctp_compute_cksum(). Just to be sure, I re-did that check
today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS
sctp_csum_check() (but I don't have a test scenario yet).
That's why I propose to fix only sctp_error() in conntrack. Regarding
IPVS, 2 out of 3 calls to sctp_compute_cksum() are preceded by
skb_make_writable(), which is correct. I can do a test for IPVS
sctp_csum_check() and check if it also needs some change, and post it in a
separate patch. Is that acceptable?
thank you in advance,
regards
--
davide
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb
2017-05-19 11:39 ` Davide Caratti
@ 2017-05-23 13:51 ` Davide Caratti
2017-05-23 19:35 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Davide Caratti @ 2017-05-23 13:51 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netfilter-devel, Florian Westphal, Marcelo Ricardo Leitner
hello Pablo,
On Fri, 2017-05-19 at 13:39 +0200, Davide Caratti wrote:
> On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote:
> > I mean, I can see other spots in the kernel tree that may be affected by this?
> > Or is it that you're only observing this from a path that is specific
> > of conntrack?
>
> I did the check before posting, and the kernel code seemed to already
> ensure skb is writable until SCTP header + sizeof(SCTP header) offset,
> before calling sctp_compute_cksum(). Just to be sure, I re-did that check
> today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS
> sctp_csum_check() (but I don't have a test scenario yet).
looking at IPVS code: it seems to me that the only call to sctp_csum_check()
is inside sctp_snat_handler(), after skb_make_writable() has returned
successfully. So, apparently misuse of sctp_compute_cksum() affects only
nf_conntrack module in sctp_error() callback.
Maybe this patch needs 'Fixes: cf6e007eef83 ("netfilter: conntrack: validate
SCTP crc32c in PREROUTING")' tag ?
thanks!
--
davide
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb
2017-05-23 13:51 ` Davide Caratti
@ 2017-05-23 19:35 ` Pablo Neira Ayuso
2017-05-23 21:29 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-05-23 19:35 UTC (permalink / raw)
To: Davide Caratti; +Cc: netfilter-devel, Florian Westphal, Marcelo Ricardo Leitner
On Tue, May 23, 2017 at 03:51:05PM +0200, Davide Caratti wrote:
> hello Pablo,
> On Fri, 2017-05-19 at 13:39 +0200, Davide Caratti wrote:
> > On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote:
> > > I mean, I can see other spots in the kernel tree that may be affected by this?
> > > Or is it that you're only observing this from a path that is specific
> > > of conntrack?
> >
> > I did the check before posting, and the kernel code seemed to already
> > ensure skb is writable until SCTP header + sizeof(SCTP header) offset,
> > before calling sctp_compute_cksum(). Just to be sure, I re-did that check
> > today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS
> > sctp_csum_check() (but I don't have a test scenario yet).
>
> looking at IPVS code: it seems to me that the only call to sctp_csum_check()
> is inside sctp_snat_handler(), after skb_make_writable() has returned
> successfully. So, apparently misuse of sctp_compute_cksum() affects only
> nf_conntrack module in sctp_error() callback.
>
> Maybe this patch needs 'Fixes: cf6e007eef83 ("netfilter: conntrack: validate
> SCTP crc32c in PREROUTING")' tag ?
Thanks for explaining.
I will append this "Fixes:" tag to this patch once I apply this.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb
2017-05-23 19:35 ` Pablo Neira Ayuso
@ 2017-05-23 21:29 ` Pablo Neira Ayuso
0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-05-23 21:29 UTC (permalink / raw)
To: Davide Caratti; +Cc: netfilter-devel, Florian Westphal, Marcelo Ricardo Leitner
On Tue, May 23, 2017 at 09:35:33PM +0200, Pablo Neira Ayuso wrote:
> On Tue, May 23, 2017 at 03:51:05PM +0200, Davide Caratti wrote:
> > hello Pablo,
> > On Fri, 2017-05-19 at 13:39 +0200, Davide Caratti wrote:
> > > On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote:
> > > > I mean, I can see other spots in the kernel tree that may be affected by this?
> > > > Or is it that you're only observing this from a path that is specific
> > > > of conntrack?
> > >
> > > I did the check before posting, and the kernel code seemed to already
> > > ensure skb is writable until SCTP header + sizeof(SCTP header) offset,
> > > before calling sctp_compute_cksum(). Just to be sure, I re-did that check
> > > today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS
> > > sctp_csum_check() (but I don't have a test scenario yet).
> >
> > looking at IPVS code: it seems to me that the only call to sctp_csum_check()
> > is inside sctp_snat_handler(), after skb_make_writable() has returned
> > successfully. So, apparently misuse of sctp_compute_cksum() affects only
> > nf_conntrack module in sctp_error() callback.
> >
> > Maybe this patch needs 'Fixes: cf6e007eef83 ("netfilter: conntrack: validate
> > SCTP crc32c in PREROUTING")' tag ?
>
> Thanks for explaining.
>
> I will append this "Fixes:" tag to this patch once I apply this.
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-23 21:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-18 16:01 [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb Davide Caratti
2017-05-19 8:41 ` Pablo Neira Ayuso
2017-05-19 11:39 ` Davide Caratti
2017-05-23 13:51 ` Davide Caratti
2017-05-23 19:35 ` Pablo Neira Ayuso
2017-05-23 21:29 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).