Re: [Patch net] mlx5: check for malformed packets
On Tue, Dec 4, 2018 at 5:16 PM Saeed Mahameed wrote: > Please understand that RX data path is really sensitive and we are > trying to find the optimal fix of any issue here, sorry for any > inconvenience. I am sorry for sending out this patch, I am certain that it wastes a lot of your time. The best we can do is to just ignore it. Thanks!
Re: [Patch net] mlx5: check for malformed packets
On Tue, 2018-12-04 at 12:21 -0800, Cong Wang wrote: > On Tue, Dec 4, 2018 at 11:33 AM Saeed Mahameed > wrote: > > On Sat, Dec 1, 2018 at 12:38 PM Cong Wang > > wrote: > > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > > > parsing IP/IPv6 headers. > > > > > > But __vlan_get_protocol() is only bound to skb->len, a malicious > > > packet could exhaust all skb->len by inserting sufficient > > > ETH_P_8021AD > > > headers, and it may not even contain an IP/IPv6 header at all, so > > > we > > > have to check if we are still safe to continue to parse IP/IPv6 > > > header. > > > If not, treat it as non-IP packet. > > > > > > This should not cause any crash as we stil have tail room in skb, > > > but we can't just rely on it either. > > > > Hi Cong, is this reproducible or just a theory ? which part of the > > code you think will cause the invalid access or crash ? > > Since you don't even read into my changelog, here it is: > > "This should not cause any crash as we stil have tail room in skb, > but we can't just rely on it either." > > As I already explained to you in a private email, when we > reference whatever field in struct iphdr, we have to make sure > the offset of that field is within skb->len. > > > > do you have steps to reproduce this? > > > > Again, you really have to read the changelog I wrote: > > > "a malicious > packet could exhaust all skb->len by inserting sufficient > ETH_P_8021AD > headers, and it may not even contain an IP/IPv6 header at all, " > I read it and i understood it, i was just wondering if you are actually able to reproduce it, and if you have the command line steps to share with us. > > > I would like to investigate this myself, it will take a couple of > > days > > if that's ok with you .. > > Sure, take your time. I am sending the patch only for showing > the problem, NOT to merge. > > > Let's discard it anyway. I am wasting my time. Ok, will be able to start looking at this in a couple of days, sorry about your time and thanks a lot for the report. Please understand that RX data path is really sensitive and we are trying to find the optimal fix of any issue here, sorry for any inconvenience. > > Thanks.
Re: [Patch net] mlx5: check for malformed packets
On Sat, Dec 1, 2018 at 12:38 PM Cong Wang wrote: > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > parsing IP/IPv6 headers. > > But __vlan_get_protocol() is only bound to skb->len, a malicious > packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD > headers, and it may not even contain an IP/IPv6 header at all, so we > have to check if we are still safe to continue to parse IP/IPv6 header. > If not, treat it as non-IP packet. > > This should not cause any crash as we stil have tail room in skb, > but we can't just rely on it either. > > Cc: Tariq Toukan > Cc: Saeed Mahameed > Signed-off-by: Cong Wang NAcked-by: Cong Wang This patch has no value for upstream. Let's discard it. Thanks!
Re: [Patch net] mlx5: check for malformed packets
On Tue, Dec 4, 2018 at 11:33 AM Saeed Mahameed wrote: > > On Sat, Dec 1, 2018 at 12:38 PM Cong Wang wrote: > > > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > > parsing IP/IPv6 headers. > > > > But __vlan_get_protocol() is only bound to skb->len, a malicious > > packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD > > headers, and it may not even contain an IP/IPv6 header at all, so we > > have to check if we are still safe to continue to parse IP/IPv6 header. > > If not, treat it as non-IP packet. > > > > This should not cause any crash as we stil have tail room in skb, > > but we can't just rely on it either. > > Hi Cong, is this reproducible or just a theory ? which part of the > code you think will cause the invalid access or crash ? Since you don't even read into my changelog, here it is: "This should not cause any crash as we stil have tail room in skb, but we can't just rely on it either." As I already explained to you in a private email, when we reference whatever field in struct iphdr, we have to make sure the offset of that field is within skb->len. > do you have steps to reproduce this? > Again, you really have to read the changelog I wrote: "a malicious packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD headers, and it may not even contain an IP/IPv6 header at all, " > I would like to investigate this myself, it will take a couple of days > if that's ok with you .. Sure, take your time. I am sending the patch only for showing the problem, NOT to merge. Let's discard it anyway. I am wasting my time. Thanks.
Re: [Patch net] mlx5: check for malformed packets
On Tue, Dec 4, 2018 at 11:28 AM Saeed Mahameed wrote: > > We are against having inline keywords in c file, so better if you > move this function to a header file an force the inline keyword there. Two points: 1. The existing code without my patch has inline keyword. Why not move it from the beginning? 2. As I already mentioned, inline doesn't work with my patch, __always_inline does. Therefore, moving inline to a header doesn't make any difference.
Re: [Patch net] mlx5: check for malformed packets
On Sat, Dec 1, 2018 at 12:38 PM Cong Wang wrote: > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > parsing IP/IPv6 headers. > > But __vlan_get_protocol() is only bound to skb->len, a malicious > packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD > headers, and it may not even contain an IP/IPv6 header at all, so we > have to check if we are still safe to continue to parse IP/IPv6 header. > If not, treat it as non-IP packet. > > This should not cause any crash as we stil have tail room in skb, > but we can't just rely on it either. Hi Cong, is this reproducible or just a theory ? which part of the code you think will cause the invalid access or crash ? do you have steps to reproduce this? I would like to investigate this myself, it will take a couple of days if that's ok with you .. Thanks, Saeed.
Re: [Patch net] mlx5: check for malformed packets
On Mon, Dec 3, 2018 at 10:39 AM Cong Wang wrote: > > On Sat, Dec 1, 2018 at 12:38 PM Cong Wang wrote: > > > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > > parsing IP/IPv6 headers. > > One thing I noticed while reviewing the assembly code is that > is_last_ethertype_ip() is no longer inlined after this patch. > > I think I should keep it inlined by using __always_inline, as it is on > a hot path. > > What do you think, Tariq? We are against having inline keywords in c file, so better if you move this function to a header file an force the inline keyword there.
Re: [Patch net] mlx5: check for malformed packets
On Sat, Dec 1, 2018 at 12:38 PM Cong Wang wrote: > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > parsing IP/IPv6 headers. One thing I noticed while reviewing the assembly code is that is_last_ethertype_ip() is no longer inlined after this patch. I think I should keep it inlined by using __always_inline, as it is on a hot path. What do you think, Tariq?
Re: [Patch net] mlx5: check for malformed packets
On Sun, Dec 2, 2018 at 9:11 PM Cong Wang wrote: > > On Sun, Dec 2, 2018 at 12:56 AM Tariq Toukan wrote: > > > > > + } else if (*proto == htons(ETH_P_IPV6)) { > > > > No need for an else here, the first if block always returns. > > > Yeah, but not sure if this makes a difference on the generated > asm code. I will give it a try anyway. I just tried, there is no difference on the generated assembly code. The gcc I use is: $ gcc --version gcc (GCC) 8.2.1 20181011 (Red Hat 8.2.1-4) Copyright (C) 2018 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Are you okay with the current code now? :)
Re: [Patch net] mlx5: check for malformed packets
On Sun, Dec 2, 2018 at 12:56 AM Tariq Toukan wrote: > > > > On 01/12/2018 10:38 PM, Cong Wang wrote: > > + if (*proto == htons(ETH_P_IP)) { > > + if (unlikely(*network_depth > skb->len - sizeof(struct > > iphdr))) > > + return false; > > + return true; > > Or just do the following? > return *network_depth <= skb->len - sizeof(struct iphdr)); > > We'll lose the compiler hint though, so I'm not sure which is better. It is very important to keep this unlikely(), as it is on a hot path. > > > + } else if (*proto == htons(ETH_P_IPV6)) { > > No need for an else here, the first if block always returns. Yeah, but not sure if this makes a difference on the generated asm code. I will give it a try anyway. Thanks.
Re: [Patch net] mlx5: check for malformed packets
On 01/12/2018 10:38 PM, Cong Wang wrote: > is_last_ethertype_ip() is used to check IP/IPv6 protocol before > parsing IP/IPv6 headers. > > But __vlan_get_protocol() is only bound to skb->len, a malicious > packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD > headers, and it may not even contain an IP/IPv6 header at all, so we > have to check if we are still safe to continue to parse IP/IPv6 header. > If not, treat it as non-IP packet. > > This should not cause any crash as we stil have tail room in skb, > but we can't just rely on it either. > > Cc: Tariq Toukan > Cc: Saeed Mahameed > Signed-off-by: Cong Wang > --- > drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) > Hi Cong, Thanks for your patch. > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > index 624eed345b5d..1e505013ebfd 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > @@ -693,7 +693,18 @@ static inline bool is_last_ethertype_ip(struct sk_buff > *skb, int *network_depth, > { > *proto = ((struct ethhdr *)skb->data)->h_proto; > *proto = __vlan_get_protocol(skb, *proto, network_depth); > - return (*proto == htons(ETH_P_IP) || *proto == htons(ETH_P_IPV6)); > + > + if (*proto == htons(ETH_P_IP)) { > + if (unlikely(*network_depth > skb->len - sizeof(struct iphdr))) > + return false; > + return true; Or just do the following? return *network_depth <= skb->len - sizeof(struct iphdr)); We'll lose the compiler hint though, so I'm not sure which is better. > + } else if (*proto == htons(ETH_P_IPV6)) { No need for an else here, the first if block always returns. > + if (unlikely(*network_depth > skb->len - sizeof(struct > ipv6hdr))) > + return false; > + return true; Same applies here. > + } > + > + return false; > } > > static inline void mlx5e_enable_ecn(struct mlx5e_rq *rq, struct sk_buff > *skb) >
[Patch net] mlx5: check for malformed packets
is_last_ethertype_ip() is used to check IP/IPv6 protocol before parsing IP/IPv6 headers. But __vlan_get_protocol() is only bound to skb->len, a malicious packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD headers, and it may not even contain an IP/IPv6 header at all, so we have to check if we are still safe to continue to parse IP/IPv6 header. If not, treat it as non-IP packet. This should not cause any crash as we stil have tail room in skb, but we can't just rely on it either. Cc: Tariq Toukan Cc: Saeed Mahameed Signed-off-by: Cong Wang --- drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c index 624eed345b5d..1e505013ebfd 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c @@ -693,7 +693,18 @@ static inline bool is_last_ethertype_ip(struct sk_buff *skb, int *network_depth, { *proto = ((struct ethhdr *)skb->data)->h_proto; *proto = __vlan_get_protocol(skb, *proto, network_depth); - return (*proto == htons(ETH_P_IP) || *proto == htons(ETH_P_IPV6)); + + if (*proto == htons(ETH_P_IP)) { + if (unlikely(*network_depth > skb->len - sizeof(struct iphdr))) + return false; + return true; + } else if (*proto == htons(ETH_P_IPV6)) { + if (unlikely(*network_depth > skb->len - sizeof(struct ipv6hdr))) + return false; + return true; + } + + return false; } static inline void mlx5e_enable_ecn(struct mlx5e_rq *rq, struct sk_buff *skb) -- 2.19.1