| Message ID | 20240924150257.1059524-3-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ab9a9a9e9647392a19e7a885b08000e89c86b535 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: two fixes for qdisc_pkt_len_init() | expand |
On Tue, Sep 24, 2024 at 03:02:57PM +0000, Eric Dumazet wrote: > One path takes care of SKB_GSO_DODGY, assuming > skb->len is bigger than hdr_len. My only comment, which you may feel free to ignore, is that we've recently merged a change to replace the term 'sanity check' in the code [1]. Given that work is being done to replace terminology in the source code, I am wondering if that same ruling applies to commit messages. If so, perhaps the title of this commit can be adjusted? [1]: https://lore.kernel.org/netdev/20240912171446.12854-1-stephen@networkplumber.org/
On Wed, Sep 25, 2024 at 7:52 PM Joe Damato <jdamato@fastly.com> wrote: > > On Tue, Sep 24, 2024 at 03:02:57PM +0000, Eric Dumazet wrote: > > One path takes care of SKB_GSO_DODGY, assuming > > skb->len is bigger than hdr_len. > > My only comment, which you may feel free to ignore, is that we've > recently merged a change to replace the term 'sanity check' in the > code [1]. > > Given that work is being done to replace terminology in the source > code, I am wondering if that same ruling applies to commit messages. > > If so, perhaps the title of this commit can be adjusted? > > [1]: https://lore.kernel.org/netdev/20240912171446.12854-1-stephen@networkplumber.org/ I guess I could write the changelog in French, to make sure it is all good. git log --oneline --grep "sanity check" | wc -l 3397 I dunno...
On Wed, Sep 25, 2024 at 08:00:08PM +0200, Eric Dumazet wrote: > On Wed, Sep 25, 2024 at 7:52 PM Joe Damato <jdamato@fastly.com> wrote: > > > > On Tue, Sep 24, 2024 at 03:02:57PM +0000, Eric Dumazet wrote: > > > One path takes care of SKB_GSO_DODGY, assuming > > > skb->len is bigger than hdr_len. > > > > My only comment, which you may feel free to ignore, is that we've > > recently merged a change to replace the term 'sanity check' in the > > code [1]. > > > > Given that work is being done to replace terminology in the source > > code, I am wondering if that same ruling applies to commit messages. > > > > If so, perhaps the title of this commit can be adjusted? > > > > [1]: https://lore.kernel.org/netdev/20240912171446.12854-1-stephen@networkplumber.org/ > > I guess I could write the changelog in French, to make sure it is all good. You could, but then I'd probably email you and remind you that the documentation explicitly says "It’s important to describe the change in plain English" [1]. > git log --oneline --grep "sanity check" | wc -l > 3397 I don't know what this means. We've done it in the past and so should continue to do it in the future? OK. > I dunno... Me either, but if code is being merged as recently as a few days ago to remove this term... [1]: https://www.kernel.org/doc/html/v6.11/process/submitting-patches.html#describe-your-changes
On Wed, 25 Sep 2024 11:24:04 -0700 Joe Damato <jdamato@fastly.com> wrote: > On Wed, Sep 25, 2024 at 08:00:08PM +0200, Eric Dumazet wrote: > > On Wed, Sep 25, 2024 at 7:52 PM Joe Damato <jdamato@fastly.com> wrote: > > > > > > On Tue, Sep 24, 2024 at 03:02:57PM +0000, Eric Dumazet wrote: > > > > One path takes care of SKB_GSO_DODGY, assuming > > > > skb->len is bigger than hdr_len. > > > > > > My only comment, which you may feel free to ignore, is that we've > > > recently merged a change to replace the term 'sanity check' in the > > > code [1]. > > > > > > Given that work is being done to replace terminology in the source > > > code, I am wondering if that same ruling applies to commit messages. > > > > > > If so, perhaps the title of this commit can be adjusted? > > > > > > [1]: https://lore.kernel.org/netdev/20240912171446.12854-1-stephen@networkplumber.org/ > > > > I guess I could write the changelog in French, to make sure it is all good. > > You could, but then I'd probably email you and remind you that the > documentation explicitly says "It’s important to describe the change > in plain English" [1]. > > > git log --oneline --grep "sanity check" | wc -l > > 3397 > > I don't know what this means. We've done it in the past and so > should continue to do it in the future? OK. > > > I dunno... > > Me either, but if code is being merged as recently as a few days ago > to remove this term... > > [1]: https://www.kernel.org/doc/html/v6.11/process/submitting-patches.html#describe-your-changes > Less concerned about commit log or subject. My patch was more about keeping non-inclusive naming out of current code base.
On Wed, Sep 25, 2024 at 8:24 PM Joe Damato <jdamato@fastly.com> wrote: > > > > git log --oneline --grep "sanity check" | wc -l > > 3397 > > I don't know what this means. We've done it in the past and so > should continue to do it in the future? OK. This means that if they are in the changelogs, they can not be removed. This is immutable stuff. Should we zap git history because of some 'bad words' that most authors/committers/reviewers were not even aware of? I would understand for stuff visible in the code (comments, error messages), but the changelogs are there and can not be changed. Who knows, maybe in 10 years 'Malicious packet.' will be very offensive, then we can remove/change the _comment_ I added in this patch.
On Wed, Sep 25, 2024 at 8:55 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Sep 25, 2024 at 8:24 PM Joe Damato <jdamato@fastly.com> wrote: > > > > > > > > git log --oneline --grep "sanity check" | wc -l > > > 3397 > > > > I don't know what this means. We've done it in the past and so > > should continue to do it in the future? OK. > > This means that if they are in the changelogs, they can not be removed. > This is immutable stuff. > Should we zap git history because of some 'bad words' that most > authors/committers/reviewers were not even aware of? > > I would understand for stuff visible in the code (comments, error messages), > but the changelogs are there and can not be changed. > > Who knows, maybe in 10 years 'Malicious packet.' will be very offensive, > then we can remove/change the _comment_ I added in this patch. BTW, I looked at https://en.wikipedia.org/wiki/Sanity_check and the non inclusive part is at the very end of it. I would suggest moving it at the beginning of the article to clearly educate all potential users.
On Wed, Sep 25, 2024 at 08:55:16PM +0200, Eric Dumazet wrote: > On Wed, Sep 25, 2024 at 8:24 PM Joe Damato <jdamato@fastly.com> wrote: > > > > > > > > git log --oneline --grep "sanity check" | wc -l > > > 3397 > > > > I don't know what this means. We've done it in the past and so > > should continue to do it in the future? OK. > > This means that if they are in the changelogs, they can not be removed. > This is immutable stuff. > Should we zap git history because of some 'bad words' that most > authors/committers/reviewers were not even aware of? I never suggested that. I suggested not adding more to the changelog and also mentioned you could feel free to ignore my comment.
On Wed, Sep 25, 2024 at 9:07 PM Joe Damato <jdamato@fastly.com> wrote: > > On Wed, Sep 25, 2024 at 08:55:16PM +0200, Eric Dumazet wrote: > > On Wed, Sep 25, 2024 at 8:24 PM Joe Damato <jdamato@fastly.com> wrote: > > > > > > > > > > > > git log --oneline --grep "sanity check" | wc -l > > > > 3397 > > > > > > I don't know what this means. We've done it in the past and so > > > should continue to do it in the future? OK. > > > > This means that if they are in the changelogs, they can not be removed. > > This is immutable stuff. > > Should we zap git history because of some 'bad words' that most > > authors/committers/reviewers were not even aware of? > > I never suggested that. I suggested not adding more to the > changelog and also mentioned you could feel free to ignore my > comment. I do not ignore comments, I learnt something new today, thanks !
On Wed, Sep 25, 2024 at 09:01:23PM +0200, Eric Dumazet wrote: > On Wed, Sep 25, 2024 at 8:55 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Wed, Sep 25, 2024 at 8:24 PM Joe Damato <jdamato@fastly.com> wrote: > > > > > > > > > > > > git log --oneline --grep "sanity check" | wc -l > > > > 3397 > > > > > > I don't know what this means. We've done it in the past and so > > > should continue to do it in the future? OK. > > > > This means that if they are in the changelogs, they can not be removed. > > This is immutable stuff. > > Should we zap git history because of some 'bad words' that most > > authors/committers/reviewers were not even aware of? > > > > I would understand for stuff visible in the code (comments, error messages), > > but the changelogs are there and can not be changed. > > > > Who knows, maybe in 10 years 'Malicious packet.' will be very offensive, > > then we can remove/change the _comment_ I added in this patch. > > BTW, I looked at https://en.wikipedia.org/wiki/Sanity_check > and the non inclusive part is at the very end of it. > > I would suggest moving it at the beginning of the article to clearly > educate all potential users. Stephen has provided what was needed, which was: guidance. The guidance appears to be that code avoid this phrase, but commit messages are of less concern. This was not clear to me before, but it is very clear now and I have learned something for future reviews.
Eric Dumazet wrote: > One path takes care of SKB_GSO_DODGY, assuming > skb->len is bigger than hdr_len. > > virtio_net_hdr_to_skb() does not fully dissect TCP headers, > it only make sure it is at least 20 bytes. > > It is possible for an user to provide a malicious 'GSO' packet, > total length of 80 bytes. > > - 20 bytes of IPv4 header > - 60 bytes TCP header > - a small gso_size like 8 > > virtio_net_hdr_to_skb() would declare this packet as a normal > GSO packet, because it would see 40 bytes of payload, > bigger than gso_size. > > We need to make detect this case to not underflow > qdisc_skb_cb(skb)->pkt_len. > > Fixes: 1def9238d4aa ("net_sched: more precise pkt_len computation") > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/core/dev.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index f2c47da79f17d5ebe6b334b63d66c84c84c519fc..35b8bcfb209bd274c81380eaf6e445641306b018 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3766,10 +3766,14 @@ static void qdisc_pkt_len_init(struct sk_buff *skb) > hdr_len += sizeof(struct udphdr); > } > > - if (shinfo->gso_type & SKB_GSO_DODGY) > - gso_segs = DIV_ROUND_UP(skb->len - hdr_len, > - shinfo->gso_size); > + if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) { > + int payload = skb->len - hdr_len; > > + /* Malicious packet. */ > + if (payload <= 0) > + return; > + gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size); > + } Especially for a malicious packet, should gso_segs be reinitialized to a sane value? As sane as feasible when other fields cannot be fully trusted.. > qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len; > } > } > -- > 2.46.0.792.g87dc391469-goog >
Willem de Bruijn wrote: > Eric Dumazet wrote: > > One path takes care of SKB_GSO_DODGY, assuming > > skb->len is bigger than hdr_len. > > > > virtio_net_hdr_to_skb() does not fully dissect TCP headers, > > it only make sure it is at least 20 bytes. > > > > It is possible for an user to provide a malicious 'GSO' packet, > > total length of 80 bytes. > > > > - 20 bytes of IPv4 header > > - 60 bytes TCP header > > - a small gso_size like 8 > > > > virtio_net_hdr_to_skb() would declare this packet as a normal > > GSO packet, because it would see 40 bytes of payload, > > bigger than gso_size. > > > > We need to make detect this case to not underflow > > qdisc_skb_cb(skb)->pkt_len. > > > > Fixes: 1def9238d4aa ("net_sched: more precise pkt_len computation") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > --- > > net/core/dev.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > index f2c47da79f17d5ebe6b334b63d66c84c84c519fc..35b8bcfb209bd274c81380eaf6e445641306b018 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -3766,10 +3766,14 @@ static void qdisc_pkt_len_init(struct sk_buff *skb) > > hdr_len += sizeof(struct udphdr); > > } > > > > - if (shinfo->gso_type & SKB_GSO_DODGY) > > - gso_segs = DIV_ROUND_UP(skb->len - hdr_len, > > - shinfo->gso_size); > > + if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) { > > + int payload = skb->len - hdr_len; > > > > + /* Malicious packet. */ > > + if (payload <= 0) > > + return; > > + gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size); > > + } > > Especially for a malicious packet, should gso_segs be reinitialized to > a sane value? As sane as feasible when other fields cannot be fully > trusted.. Never mind. I guess the best thing we can do is to leave pkt_len as skb->len, indeed. Reviewed-by: Willem de Bruijn <willemb@google.com> > > qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len; > > } > > } > > -- > > 2.46.0.792.g87dc391469-goog > > > >
On Thu, Sep 26, 2024 at 11:17 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Willem de Bruijn wrote: > > Eric Dumazet wrote: > > > One path takes care of SKB_GSO_DODGY, assuming > > > skb->len is bigger than hdr_len. > > > > > > virtio_net_hdr_to_skb() does not fully dissect TCP headers, > > > it only make sure it is at least 20 bytes. > > > > > > It is possible for an user to provide a malicious 'GSO' packet, > > > total length of 80 bytes. > > > > > > - 20 bytes of IPv4 header > > > - 60 bytes TCP header > > > - a small gso_size like 8 > > > > > > virtio_net_hdr_to_skb() would declare this packet as a normal > > > GSO packet, because it would see 40 bytes of payload, > > > bigger than gso_size. > > > > > > We need to make detect this case to not underflow > > > qdisc_skb_cb(skb)->pkt_len. > > > > > > Fixes: 1def9238d4aa ("net_sched: more precise pkt_len computation") > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > > --- > > > net/core/dev.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > index f2c47da79f17d5ebe6b334b63d66c84c84c519fc..35b8bcfb209bd274c81380eaf6e445641306b018 100644 > > > --- a/net/core/dev.c > > > +++ b/net/core/dev.c > > > @@ -3766,10 +3766,14 @@ static void qdisc_pkt_len_init(struct sk_buff *skb) > > > hdr_len += sizeof(struct udphdr); > > > } > > > > > > - if (shinfo->gso_type & SKB_GSO_DODGY) > > > - gso_segs = DIV_ROUND_UP(skb->len - hdr_len, > > > - shinfo->gso_size); > > > + if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) { > > > + int payload = skb->len - hdr_len; > > > > > > + /* Malicious packet. */ > > > + if (payload <= 0) > > > + return; > > > + gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size); > > > + } > > > > Especially for a malicious packet, should gso_segs be reinitialized to > > a sane value? As sane as feasible when other fields cannot be fully > > trusted.. > > Never mind. I guess the best thing we can do is to leave pkt_len as > skb->len, indeed. > It is unclear if we can change a field in skb here, I hope that in the future we can make virtio_net_hdr_to_skb() safer. Role of qdisc_pkt_len_init() is to set a private skb->cb[] field for qdisc layer. Thanks !
On 9/26/24 11:19, Eric Dumazet wrote: > On Thu, Sep 26, 2024 at 11:17 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: >> >> Willem de Bruijn wrote: >>> Eric Dumazet wrote: >>>> One path takes care of SKB_GSO_DODGY, assuming >>>> skb->len is bigger than hdr_len. >>>> >>>> virtio_net_hdr_to_skb() does not fully dissect TCP headers, >>>> it only make sure it is at least 20 bytes. >>>> >>>> It is possible for an user to provide a malicious 'GSO' packet, >>>> total length of 80 bytes. >>>> >>>> - 20 bytes of IPv4 header >>>> - 60 bytes TCP header >>>> - a small gso_size like 8 >>>> >>>> virtio_net_hdr_to_skb() would declare this packet as a normal >>>> GSO packet, because it would see 40 bytes of payload, >>>> bigger than gso_size. >>>> >>>> We need to make detect this case to not underflow >>>> qdisc_skb_cb(skb)->pkt_len. >>>> >>>> Fixes: 1def9238d4aa ("net_sched: more precise pkt_len computation") >>>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>>> --- >>>> net/core/dev.c | 10 +++++++--- >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/net/core/dev.c b/net/core/dev.c >>>> index f2c47da79f17d5ebe6b334b63d66c84c84c519fc..35b8bcfb209bd274c81380eaf6e445641306b018 100644 >>>> --- a/net/core/dev.c >>>> +++ b/net/core/dev.c >>>> @@ -3766,10 +3766,14 @@ static void qdisc_pkt_len_init(struct sk_buff *skb) >>>> hdr_len += sizeof(struct udphdr); >>>> } >>>> >>>> - if (shinfo->gso_type & SKB_GSO_DODGY) >>>> - gso_segs = DIV_ROUND_UP(skb->len - hdr_len, >>>> - shinfo->gso_size); >>>> + if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) { >>>> + int payload = skb->len - hdr_len; >>>> >>>> + /* Malicious packet. */ >>>> + if (payload <= 0) >>>> + return; >>>> + gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size); >>>> + } >>> >>> Especially for a malicious packet, should gso_segs be reinitialized to >>> a sane value? As sane as feasible when other fields cannot be fully >>> trusted.. >> >> Never mind. I guess the best thing we can do is to leave pkt_len as >> skb->len, indeed. > > It is unclear if we can change a field in skb here, I hope that in the > future we can make virtio_net_hdr_to_skb() safer. A problem with virtio_net_hdr_to_skb() is that is needs to cope with existing implementation bending the virtio spec to its limits, and the spec definition allowed for some 'fun'. Sanitize everything is likely to break existing users. On a somewhat related note, I'm trying to push a new virtio-spec GSO feature (UDP tunnel support) that will inevitably increase the attack surface. On the flip side the general idea is to be as strict as possible with the definition of the new allowed gso types, to try to avoid the above kind of scenarios (for such gso types). The above just FYI, Paolo
diff --git a/net/core/dev.c b/net/core/dev.c index f2c47da79f17d5ebe6b334b63d66c84c84c519fc..35b8bcfb209bd274c81380eaf6e445641306b018 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3766,10 +3766,14 @@ static void qdisc_pkt_len_init(struct sk_buff *skb) hdr_len += sizeof(struct udphdr); } - if (shinfo->gso_type & SKB_GSO_DODGY) - gso_segs = DIV_ROUND_UP(skb->len - hdr_len, - shinfo->gso_size); + if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) { + int payload = skb->len - hdr_len; + /* Malicious packet. */ + if (payload <= 0) + return; + gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size); + } qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len; } }
One path takes care of SKB_GSO_DODGY, assuming skb->len is bigger than hdr_len. virtio_net_hdr_to_skb() does not fully dissect TCP headers, it only make sure it is at least 20 bytes. It is possible for an user to provide a malicious 'GSO' packet, total length of 80 bytes. - 20 bytes of IPv4 header - 60 bytes TCP header - a small gso_size like 8 virtio_net_hdr_to_skb() would declare this packet as a normal GSO packet, because it would see 40 bytes of payload, bigger than gso_size. We need to make detect this case to not underflow qdisc_skb_cb(skb)->pkt_len. Fixes: 1def9238d4aa ("net_sched: more precise pkt_len computation") Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/core/dev.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)