Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
yes, tipc_l2_device_event() only change MTU of bearer rather than the MTU of link, tipc_enable_l2_media() will be the right place to test a tiny MTU. Qian Sent from my iPhone On 5 Nov 2016, at 00:00, Jon Maloy wrote: >> -Original Message- >> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] >> On Behalf Of ?? >> Sent: Friday, 04 November, 2016 03:24 >> To: Jon Maloy ; Ben Hutchings >> ; Ying Xue >> Cc: netdev@vger.kernel.org; Eric Dumazet >> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() >> >> Hi, >> I think both tipc_l2_device_event() and tipc_enable_l2_media() need to >> refuse a >> tiny MTU for TIPC bearers. > > Right, except that when looking into the code for tipc_l2_device_event() I > realize that it currently doesn't try to re-adapt to a new MTU at all. It > just calls tipc_reset_bearer(), which I suspect has changed somewhere along > the road to ignore the MTU. So, you only need to change > tipc_enable_l2_media(). > > ///jon > >> >> tipc_l2_device_event() used to update the TIPC MTU value when executing a >> command like 'ifconfig eth0 MTU 1 up'. >> tipc_enable_l2_media() will be invoked when the TIPC network created. >> >> Thanks. >> >> Qian Zhang >> MarvelTeam Qihoo 360 >> >> >> >> -----邮件原件- >> 发件人: Jon Maloy [mailto:jon.ma...@ericsson.com] >> 发送时间: 2016年11月1日 19:37 >> 收件人: 张谦; Ben Hutchings; Ying Xue >> 抄送: netdev@vger.kernel.org; Eric Dumazet >> 主题: RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() >> >> Hi, >> I think we all agreed in the end that this is a possible, but highly >> implausible, >> scenario, and rather as a point of exploit than a functional bug. >> The solution is very simple, and described further down in this mail thread. >> I have >> not done anything to it yet, but you are welcome to contribute. >> >> BR >> ///jon >> >> >>> -Original Message- >>> From: 张谦 [mailto:zhangqia...@360.cn] >>> Sent: Tuesday, 01 November, 2016 02:35 >>> To: Ben Hutchings ; Jon Maloy >>> ; Ying Xue >>> Cc: netdev@vger.kernel.org; Eric Dumazet >>> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in >>> tipc_msg_build() >>> >>> Hi all, >>> I have accomplished a PoC can help you to confirm this issue. >>> >>> And two weeks passed from the last mail, can you tell me the progress >>> of the patch to this flaw? >>> >>> Thanks. >>> >>> Qian Zhang >>> Marvel Team Qihoo 360 >>> >>> >>> -邮件原件- >>> 发件人: Ben Hutchings [mailto:b...@decadent.org.uk] >>> 发送时间: 2016年10月21日 23:00 >>> 收件人: Jon Maloy; Ying Xue >>> 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet >>> 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() >>> >>> On Fri, 2016-10-21 at 14:57 +, Jon Maloy wrote: >>>>> -Original Message- >>>>>>> From: Ben Hutchings [mailto:b...@decadent.org.uk] >>>>> Sent: Thursday, 20 October, 2016 12:40 >>>>>>> To: Jon Maloy ; Ying Xue >>>>>>> >>>>>>>>> Cc: netdev@vger.kernel.org; Qian Zhang >>>>>>>>> ; Eric Dumazet >>>>>>> >>>>> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in >>>>> tipc_msg_build() >>>>> >>>>> On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: >>>>> [...] >>>>>>> At this point we're about to copy INT_H_SIZE + mhsz bytes into >>>>>>> the first fragment. If that's already limited to be less than >>>>>>> or equal to MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. >>>>>>> But if >>>>> >>>>> MAX_H_SIZE >>>>>>> is the maximum value of mhsz, that won't be good enough. >>>>>> >>>>>> >>>>>> >>>>>> MAX_H_SIZE is 60 bytes, but in practice you will never see an >>>>>> mhsz larger than >>>>> >>>>> the biggest header we are actually using, which is MCAST_H_SIZE >>>>> (==44 >>> bytes). >>>>>> INT_H_SIZE is 40 bytes, so you are in reality testing for >>>>>> whether we have an mtu >>>>> >>>>> < 84 bytes. >>>>>> You won't find any interfaces or protocols that come even close >>>>>> to this >>>>> >>>>> limitation, so to me this test is redundant. >>>>> >>>>> But I can easily create such an interface: >>>>> >>>>> $ unshare -n -U -r >>>>> # ip l set lo mtu 1 >>>>> >>>>> Ben. >>>> >>>> >>>> It won't be very useful though. But I assume you mean it could be a >>>> possible exploit, >>> >>> Exactly. >>> >>>> and I suspect a few other things would break both in TIPC and in >>>> other stacks if you do anything like that. I think the solution to >>>> this is not to fix all possible places in the code where this can go >>>> wrong, but rather to have a generic test where we refuse to attach >>>> bearers/interfaces offering an mtu < e.g. 1000 bytes. This can >>>> easily be done in tipc_enable_l2_media(). >>> >>> Yes. >>> >>> Ben. >>> >>> -- >>> Ben Hutchings >>> One of the nice things about standards is that there are so many of them. >
RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> -Original Message- > From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] > On Behalf Of ?? > Sent: Friday, 04 November, 2016 03:24 > To: Jon Maloy ; Ben Hutchings > ; Ying Xue > Cc: netdev@vger.kernel.org; Eric Dumazet > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > Hi, > I think both tipc_l2_device_event() and tipc_enable_l2_media() need to refuse > a > tiny MTU for TIPC bearers. Right, except that when looking into the code for tipc_l2_device_event() I realize that it currently doesn't try to re-adapt to a new MTU at all. It just calls tipc_reset_bearer(), which I suspect has changed somewhere along the road to ignore the MTU. So, you only need to change tipc_enable_l2_media(). ///jon > > tipc_l2_device_event() used to update the TIPC MTU value when executing a > command like 'ifconfig eth0 MTU 1 up'. > tipc_enable_l2_media() will be invoked when the TIPC network created. > > Thanks. > > Qian Zhang > MarvelTeam Qihoo 360 > > > > -邮件原件- > 发件人: Jon Maloy [mailto:jon.ma...@ericsson.com] > 发送时间: 2016年11月1日 19:37 > 收件人: 张谦; Ben Hutchings; Ying Xue > 抄送: netdev@vger.kernel.org; Eric Dumazet > 主题: RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > Hi, > I think we all agreed in the end that this is a possible, but highly > implausible, > scenario, and rather as a point of exploit than a functional bug. > The solution is very simple, and described further down in this mail thread. > I have > not done anything to it yet, but you are welcome to contribute. > > BR > ///jon > > > > -Original Message- > > From: 张谦 [mailto:zhangqia...@360.cn] > > Sent: Tuesday, 01 November, 2016 02:35 > > To: Ben Hutchings ; Jon Maloy > > ; Ying Xue > > Cc: netdev@vger.kernel.org; Eric Dumazet > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in > > tipc_msg_build() > > > > Hi all, > > I have accomplished a PoC can help you to confirm this issue. > > > > And two weeks passed from the last mail, can you tell me the progress > > of the patch to this flaw? > > > > Thanks. > > > > Qian Zhang > > Marvel Team Qihoo 360 > > > > > > -邮件原件- > > 发件人: Ben Hutchings [mailto:b...@decadent.org.uk] > > 发送时间: 2016年10月21日 23:00 > > 收件人: Jon Maloy; Ying Xue > > 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet > > 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > > > On Fri, 2016-10-21 at 14:57 +, Jon Maloy wrote: > > > > -Original Message- > > > > > > From: Ben Hutchings [mailto:b...@decadent.org.uk] > > > > Sent: Thursday, 20 October, 2016 12:40 > > > > > > To: Jon Maloy ; Ying Xue > > > > > > > > > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang > > > > > > > > ; Eric Dumazet > > > > > > > > > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in > > > > tipc_msg_build() > > > > > > > > On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: > > > > [...] > > > > > > At this point we're about to copy INT_H_SIZE + mhsz bytes into > > > > > > the first fragment. If that's already limited to be less than > > > > > > or equal to MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. > > > > > > But if > > > > > > > > MAX_H_SIZE > > > > > > is the maximum value of mhsz, that won't be good enough. > > > > > > > > > > > > > > > > > > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an > > > > > mhsz larger than > > > > > > > > the biggest header we are actually using, which is MCAST_H_SIZE > > > > (==44 > > bytes). > > > > > INT_H_SIZE is 40 bytes, so you are in reality testing for > > > > > whether we have an mtu > > > > > > > > < 84 bytes. > > > > > You won't find any interfaces or protocols that come even close > > > > > to this > > > > > > > > limitation, so to me this test is redundant. > > > > > > > > But I can easily create such an interface: > > > > > > > > $ unshare -n -U -r > > > > # ip l set lo mtu 1 > > > > > > > > Ben. > > > > > > > > > It won't be very useful though. But I assume you mean it could be a > > > possible exploit, > > > > Exactly. > > > > > and I suspect a few other things would break both in TIPC and in > > > other stacks if you do anything like that. I think the solution to > > > this is not to fix all possible places in the code where this can go > > > wrong, but rather to have a generic test where we refuse to attach > > > bearers/interfaces offering an mtu < e.g. 1000 bytes. This can > > > easily be done in tipc_enable_l2_media(). > > > > Yes. > > > > Ben. > > > > -- > > Ben Hutchings > > One of the nice things about standards is that there are so many of them.
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
Hi, I think both tipc_l2_device_event() and tipc_enable_l2_media() need to refuse a tiny MTU for TIPC bearers. tipc_l2_device_event() used to update the TIPC MTU value when executing a command like 'ifconfig eth0 MTU 1 up'. tipc_enable_l2_media() will be invoked when the TIPC network created. Thanks. Qian Zhang MarvelTeam Qihoo 360 -邮件原件- 发件人: Jon Maloy [mailto:jon.ma...@ericsson.com] 发送时间: 2016年11月1日 19:37 收件人: 张谦; Ben Hutchings; Ying Xue 抄送: netdev@vger.kernel.org; Eric Dumazet 主题: RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() Hi, I think we all agreed in the end that this is a possible, but highly implausible, scenario, and rather as a point of exploit than a functional bug. The solution is very simple, and described further down in this mail thread. I have not done anything to it yet, but you are welcome to contribute. BR ///jon > -Original Message- > From: 张谦 [mailto:zhangqia...@360.cn] > Sent: Tuesday, 01 November, 2016 02:35 > To: Ben Hutchings ; Jon Maloy > ; Ying Xue > Cc: netdev@vger.kernel.org; Eric Dumazet > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in > tipc_msg_build() > > Hi all, > I have accomplished a PoC can help you to confirm this issue. > > And two weeks passed from the last mail, can you tell me the progress > of the patch to this flaw? > > Thanks. > > Qian Zhang > Marvel Team Qihoo 360 > > > -邮件原件- > 发件人: Ben Hutchings [mailto:b...@decadent.org.uk] > 发送时间: 2016年10月21日 23:00 > 收件人: Jon Maloy; Ying Xue > 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet > 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > On Fri, 2016-10-21 at 14:57 +, Jon Maloy wrote: > > > -Original Message- > > > > > From: Ben Hutchings [mailto:b...@decadent.org.uk] > > > Sent: Thursday, 20 October, 2016 12:40 > > > > > To: Jon Maloy ; Ying Xue > > > > > > > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang > > > > > > > ; Eric Dumazet > > > > > > > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in > > > tipc_msg_build() > > > > > > On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: > > > [...] > > > > > At this point we're about to copy INT_H_SIZE + mhsz bytes into > > > > > the first fragment. If that's already limited to be less than > > > > > or equal to MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. > > > > > But if > > > > > > MAX_H_SIZE > > > > > is the maximum value of mhsz, that won't be good enough. > > > > > > > > > > > > > > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an > > > > mhsz larger than > > > > > > the biggest header we are actually using, which is MCAST_H_SIZE > > > (==44 > bytes). > > > > INT_H_SIZE is 40 bytes, so you are in reality testing for > > > > whether we have an mtu > > > > > > < 84 bytes. > > > > You won't find any interfaces or protocols that come even close > > > > to this > > > > > > limitation, so to me this test is redundant. > > > > > > But I can easily create such an interface: > > > > > > $ unshare -n -U -r > > > # ip l set lo mtu 1 > > > > > > Ben. > > > > > > It won't be very useful though. But I assume you mean it could be a > > possible exploit, > > Exactly. > > > and I suspect a few other things would break both in TIPC and in > > other stacks if you do anything like that. I think the solution to > > this is not to fix all possible places in the code where this can go > > wrong, but rather to have a generic test where we refuse to attach > > bearers/interfaces offering an mtu < e.g. 1000 bytes. This can > > easily be done in tipc_enable_l2_media(). > > Yes. > > Ben. > > -- > Ben Hutchings > One of the nice things about standards is that there are so many of them.
RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
Hi, I think we all agreed in the end that this is a possible, but highly implausible, scenario, and rather as a point of exploit than a functional bug. The solution is very simple, and described further down in this mail thread. I have not done anything to it yet, but you are welcome to contribute. BR ///jon > -Original Message- > From: 张谦 [mailto:zhangqia...@360.cn] > Sent: Tuesday, 01 November, 2016 02:35 > To: Ben Hutchings ; Jon Maloy > ; Ying Xue > Cc: netdev@vger.kernel.org; Eric Dumazet > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > Hi all, > I have accomplished a PoC can help you to confirm this issue. > > And two weeks passed from the last mail, can you tell me the progress of the > patch to this flaw? > > Thanks. > > Qian Zhang > Marvel Team Qihoo 360 > > > -邮件原件- > 发件人: Ben Hutchings [mailto:b...@decadent.org.uk] > 发送时间: 2016年10月21日 23:00 > 收件人: Jon Maloy; Ying Xue > 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet > 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > On Fri, 2016-10-21 at 14:57 +, Jon Maloy wrote: > > > -Original Message- > > > > > From: Ben Hutchings [mailto:b...@decadent.org.uk] > > > Sent: Thursday, 20 October, 2016 12:40 > > > > > To: Jon Maloy ; Ying Xue > > > > > > > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang ; > > > > > > > Eric Dumazet > > > > > > > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in > > > tipc_msg_build() > > > > > > On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: > > > [...] > > > > > At this point we're about to copy INT_H_SIZE + mhsz bytes into > > > > > the first fragment. If that's already limited to be less than > > > > > or equal to MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. > > > > > But if > > > > > > MAX_H_SIZE > > > > > is the maximum value of mhsz, that won't be good enough. > > > > > > > > > > > > > > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz > > > > larger than > > > > > > the biggest header we are actually using, which is MCAST_H_SIZE (==44 > bytes). > > > > INT_H_SIZE is 40 bytes, so you are in reality testing for whether > > > > we have an mtu > > > > > > < 84 bytes. > > > > You won't find any interfaces or protocols that come even close to > > > > this > > > > > > limitation, so to me this test is redundant. > > > > > > But I can easily create such an interface: > > > > > > $ unshare -n -U -r > > > # ip l set lo mtu 1 > > > > > > Ben. > > > > > > It won't be very useful though. But I assume you mean it could be a > > possible exploit, > > Exactly. > > > and I suspect a few other things would break both in TIPC and in > > other stacks if you do anything like that. I think the solution to > > this is not to fix all possible places in the code where this can go > > wrong, but rather to have a generic test where we refuse to attach > > bearers/interfaces offering an mtu < e.g. 1000 bytes. This can easily > > be done in tipc_enable_l2_media(). > > Yes. > > Ben. > > -- > Ben Hutchings > One of the nice things about standards is that there are so many of them.
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Fri, 2016-10-21 at 14:57 +, Jon Maloy wrote: > > -Original Message- > > > > From: Ben Hutchings [mailto:b...@decadent.org.uk] > > Sent: Thursday, 20 October, 2016 12:40 > > > > To: Jon Maloy ; Ying Xue > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang ; Eric > > > > > > Dumazet > > > > > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > > > On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: > > [...] > > > > At this point we're about to copy INT_H_SIZE + mhsz bytes into the > > > > first fragment. If that's already limited to be less than or equal to > > > > MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if > > > > MAX_H_SIZE > > > > is the maximum value of mhsz, that won't be good enough. > > > > > > > > > > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger > > > than > > > > the biggest header we are actually using, which is MCAST_H_SIZE (==44 > > bytes). > > > INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have > > > an mtu > > > > < 84 bytes. > > > You won't find any interfaces or protocols that come even close to this > > > > limitation, so to me this test is redundant. > > > > But I can easily create such an interface: > > > > $ unshare -n -U -r > > # ip l set lo mtu 1 > > > > Ben. > > > It won't be very useful though. But I assume you mean it could be a > possible exploit, Exactly. > and I suspect a few other things would break both in TIPC and in > other stacks if you do anything like that. I think the solution to > this is not to fix all possible places in the code where this can go > wrong, but rather to have a generic test where we refuse to attach > bearers/interfaces offering an mtu < e.g. 1000 bytes. This can easily > be done in tipc_enable_l2_media(). Yes. Ben. -- Ben Hutchings One of the nice things about standards is that there are so many of them. signature.asc Description: This is a digitally signed message part
RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> -Original Message- > From: Ben Hutchings [mailto:b...@decadent.org.uk] > Sent: Thursday, 20 October, 2016 12:40 > To: Jon Maloy ; Ying Xue > Cc: netdev@vger.kernel.org; Qian Zhang ; Eric Dumazet > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: > [...] > > > At this point we're about to copy INT_H_SIZE + mhsz bytes into the > > > first fragment. If that's already limited to be less than or equal to > > > MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if > MAX_H_SIZE > > > is the maximum value of mhsz, that won't be good enough. > > > > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger > > than > the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes). > > INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have > > an mtu > < 84 bytes. > > You won't find any interfaces or protocols that come even close to this > limitation, so to me this test is redundant. > > But I can easily create such an interface: > > $ unshare -n -U -r > # ip l set lo mtu 1 > > Ben. It won't be very useful though. But I assume you mean it could be a possible exploit, and I suspect a few other things would break both in TIPC and in other stacks if you do anything like that. I think the solution to this is not to fix all possible places in the code where this can go wrong, but rather to have a generic test where we refuse to attach bearers/interfaces offering an mtu < e.g. 1000 bytes. This can easily be done in tipc_enable_l2_media(). ///jon > > -- > Ben Hutchings > Never put off till tomorrow what you can avoid all together.
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: [...] > > At this point we're about to copy INT_H_SIZE + mhsz bytes into the > > first fragment. If that's already limited to be less than or equal to > > MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE > > is the maximum value of mhsz, that won't be good enough. > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger > than the biggest header we are actually using, which is MCAST_H_SIZE (==44 > bytes). > INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an > mtu < 84 bytes. > You won't find any interfaces or protocols that come even close to this > limitation, so to me this test is redundant. But I can easily create such an interface: $ unshare -n -U -r # ip l set lo mtu 1 Ben. -- Ben Hutchings Never put off till tomorrow what you can avoid all together. signature.asc Description: This is a digitally signed message part
RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> -Original Message- > From: Ben Hutchings [mailto:b...@decadent.org.uk] > Sent: Thursday, 20 October, 2016 08:46 > To: Ying Xue ; Jon Maloy > Cc: netdev@vger.kernel.org; Qian Zhang ; Eric Dumazet > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote: > > On 10/19/2016 10:16 AM, Ben Hutchings wrote: > > > Qian Zhang (张谦) reported a potential socket buffer overflow in > > > tipc_msg_build(). The minimum fragment length needs to be checked > > > against the maximum packet size, which is based on the link MTU. > [...] > > > > > > --- a/net/tipc/msg.c > > > +++ b/net/tipc/msg.c > > > @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct > msghdr *m, > > > > > goto error; > > > > > } > > > > > > > > + /* Check that fragment and message header will fit */ > > > > > + if (INT_H_SIZE + mhsz > pktmax) > > > + return -EMSGSIZE; > > > > > > The "mhsz" represents the size of tipc packet header for current socket, > > INT_H_SIZE indicates the size of tipc internal message header. So it > > seems unreasonable to identify whether the sum of both header sizes is > > bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to > > compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should > > return EMSGSIZE error code. > > At this point we're about to copy INT_H_SIZE + mhsz bytes into the > first fragment. If that's already limited to be less than or equal to > MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE > is the maximum value of mhsz, that won't be good enough. MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger than the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes). INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu < 84 bytes. You won't find any interfaces or protocols that come even close to this limitation, so to me this test is redundant. Regards ///jon > > Ben. > > > > + > > > > > /* Prepare reusable fragment header */ > > > > > tipc_msg_init(msg_prevnode(mhdr), &pkthdr, > MSG_FRAGMENTER, > > > > > FIRST_FRAGMENT, INT_H_SIZE, > msg_destnode(mhdr)); > > > > > > > > > > -- > Ben Hutchings > Never put off till tomorrow what you can avoid all together.
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote: > On 10/19/2016 10:16 AM, Ben Hutchings wrote: > > Qian Zhang (张谦) reported a potential socket buffer overflow in > > tipc_msg_build(). The minimum fragment length needs to be checked > > against the maximum packet size, which is based on the link MTU. [...] > > > > --- a/net/tipc/msg.c > > +++ b/net/tipc/msg.c > > @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct > > msghdr *m, > > > > goto error; > > > > } > > > > > > + /* Check that fragment and message header will fit */ > > > > + if (INT_H_SIZE + mhsz > pktmax) > > + return -EMSGSIZE; > > > The "mhsz" represents the size of tipc packet header for current socket, > INT_H_SIZE indicates the size of tipc internal message header. So it > seems unreasonable to identify whether the sum of both header sizes is > bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to > compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should > return EMSGSIZE error code. At this point we're about to copy INT_H_SIZE + mhsz bytes into the first fragment. If that's already limited to be less than or equal to MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE is the maximum value of mhsz, that won't be good enough. Ben. > > + > > > > /* Prepare reusable fragment header */ > > > > tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER, > > > > FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr)); > > > > > -- Ben Hutchings Never put off till tomorrow what you can avoid all together. signature.asc Description: This is a digitally signed message part
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On 10/19/2016 10:16 AM, Ben Hutchings wrote: > Qian Zhang (张谦) reported a potential socket buffer overflow in > tipc_msg_build(). The minimum fragment length needs to be checked > against the maximum packet size, which is based on the link MTU. > > Reported-by: Qian Zhang (张谦) > Signed-off-by: Ben Hutchings > --- > This is untested, but I think it fixes the issue reported. Ideally > tipc_l2_device_event() would also disable use of TIPC on devices with > too small an MTU, like several other protocols do. > Yes, I think so. I will create a patch to disable TIPC sending process when MTU size is too small. > Ben. > > net/tipc/msg.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/net/tipc/msg.c b/net/tipc/msg.c > index 17201aa8423d..b9124ac82c29 100644 > --- a/net/tipc/msg.c > +++ b/net/tipc/msg.c > @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr > *m, > goto error; > } > > + /* Check that fragment and message header will fit */ > + if (INT_H_SIZE + mhsz > pktmax) > + return -EMSGSIZE; The "mhsz" represents the size of tipc packet header for current socket, INT_H_SIZE indicates the size of tipc internal message header. So it seems unreasonable to identify whether the sum of both header sizes is bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should return EMSGSIZE error code. > + > /* Prepare reusable fragment header */ > tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER, > FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr)); >
[PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
Qian Zhang (张谦) reported a potential socket buffer overflow in tipc_msg_build(). The minimum fragment length needs to be checked against the maximum packet size, which is based on the link MTU. Reported-by: Qian Zhang (张谦) Signed-off-by: Ben Hutchings --- This is untested, but I think it fixes the issue reported. Ideally tipc_l2_device_event() would also disable use of TIPC on devices with too small an MTU, like several other protocols do. Ben. net/tipc/msg.c | 4 1 file changed, 4 insertions(+) diff --git a/net/tipc/msg.c b/net/tipc/msg.c index 17201aa8423d..b9124ac82c29 100644 --- a/net/tipc/msg.c +++ b/net/tipc/msg.c @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, goto error; } + /* Check that fragment and message header will fit */ + if (INT_H_SIZE + mhsz > pktmax) + return -EMSGSIZE; + /* Prepare reusable fragment header */ tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER, FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr)); signature.asc Description: Digital signature
