Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

2016-11-04 Thread 张谦
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()

2016-11-04 Thread Jon Maloy
> -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()

2016-11-04 Thread 张谦
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()

2016-11-01 Thread Jon Maloy
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()

2016-10-21 Thread Ben Hutchings
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()

2016-10-21 Thread Jon Maloy


> -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()

2016-10-20 Thread Ben Hutchings
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()

2016-10-20 Thread Jon Maloy


> -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()

2016-10-20 Thread Ben Hutchings
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()

2016-10-20 Thread Ying Xue
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));
>