[PATCH net-next 0/2] net: control the length of the altname list

[PATCH net-next 0/2] net: control the length of the altname list

[Jakub Kicinski]

Count the memory used for altnames and don't let user
overflow the property nlattr. This was reported by George:
https://lore.kernel.org/all/3e564baf-a1dd-122e-2882-ff143f7eb578@gmail.com/

Targeting net-next because I think we generally don't consider
tightening "root controlled" memory accounting to be a fix.
There's also some risk of breakage.

Jakub Kicinski (2):
  net: account alternate interface name memory
  net: limit altnames to 64k total

 net/core/rtnetlink.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH net-next 1/2] net: account alternate interface name memory

[Jakub Kicinski]

George reports that altnames can eat up kernel memory.
We should charge that memory appropriately.

Reported-by: George Shuklin <george.shuklin@gmail.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a759f9e0a847..aa05e89cc47c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3658,7 +3658,7 @@ static int rtnl_alt_ifname(int cmd, struct net_device *dev, struct nlattr *attr,
 	if (err)
 		return err;
 
-	alt_ifname = nla_strdup(attr, GFP_KERNEL);
+	alt_ifname = nla_strdup(attr, GFP_KERNEL_ACCOUNT);
 	if (!alt_ifname)
 		return -ENOMEM;
 
-- 
2.34.1

[PATCH net-next 2/2] net: limit altnames to 64k total

[Jakub Kicinski]

Property list (altname is a link "property") is wrapped
in a nlattr. nlattrs length is 16bit so practically
speaking the list of properties can't be longer than
that, otherwise user space would have to interpret
broken netlink messages.

Prevent the problem from occurring by checking the length
of the property list before adding new entries.

Reported-by: George Shuklin <george.shuklin@gmail.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/rtnetlink.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index aa05e89cc47c..159c9c61e6af 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3652,12 +3652,23 @@ static int rtnl_alt_ifname(int cmd, struct net_device *dev, struct nlattr *attr,
 			   bool *changed, struct netlink_ext_ack *extack)
 {
 	char *alt_ifname;
+	size_t size;
 	int err;
 
 	err = nla_validate(attr, attr->nla_len, IFLA_MAX, ifla_policy, extack);
 	if (err)
 		return err;
 
+	if (cmd == RTM_NEWLINKPROP) {
+		size = rtnl_prop_list_size(dev);
+		size += nla_total_size(ALTIFNAMSIZ);
+		if (size >= U16_MAX) {
+			NL_SET_ERR_MSG(extack,
+				       "effective property list too long");
+			return -EINVAL;
+		}
+	}
+
 	alt_ifname = nla_strdup(attr, GFP_KERNEL_ACCOUNT);
 	if (!alt_ifname)
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH net-next 2/2] net: limit altnames to 64k total

[David Ahern]

On 3/9/22 11:29 AM, Jakub Kicinski wrote:
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index aa05e89cc47c..159c9c61e6af 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3652,12 +3652,23 @@ static int rtnl_alt_ifname(int cmd, struct net_device *dev, struct nlattr *attr,
>  			   bool *changed, struct netlink_ext_ack *extack)
>  {
>  	char *alt_ifname;
> +	size_t size;
>  	int err;
>  
>  	err = nla_validate(attr, attr->nla_len, IFLA_MAX, ifla_policy, extack);
>  	if (err)
>  		return err;
>  
> +	if (cmd == RTM_NEWLINKPROP) {
> +		size = rtnl_prop_list_size(dev);
> +		size += nla_total_size(ALTIFNAMSIZ);
> +		if (size >= U16_MAX) {
> +			NL_SET_ERR_MSG(extack,
> +				       "effective property list too long");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	alt_ifname = nla_strdup(attr, GFP_KERNEL_ACCOUNT);
>  	if (!alt_ifname)
>  		return -ENOMEM;

this tests the existing property size. Don't you want to test the size
with the alt_ifname - does it make the list go over 64kB?

Re: [PATCH net-next 2/2] net: limit altnames to 64k total

[Jakub Kicinski]

On Wed, 9 Mar 2022 19:51:07 -0700 David Ahern wrote:
> On 3/9/22 11:29 AM, Jakub Kicinski wrote:
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index aa05e89cc47c..159c9c61e6af 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -3652,12 +3652,23 @@ static int rtnl_alt_ifname(int cmd, struct net_device *dev, struct nlattr *attr,
> >  			   bool *changed, struct netlink_ext_ack *extack)
> >  {
> >  	char *alt_ifname;
> > +	size_t size;
> >  	int err;
> >  
> >  	err = nla_validate(attr, attr->nla_len, IFLA_MAX, ifla_policy, extack);
> >  	if (err)
> >  		return err;
> >  
> > +	if (cmd == RTM_NEWLINKPROP) {
> > +		size = rtnl_prop_list_size(dev);
> > +		size += nla_total_size(ALTIFNAMSIZ);
> > +		if (size >= U16_MAX) {
> > +			NL_SET_ERR_MSG(extack,
> > +				       "effective property list too long");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	alt_ifname = nla_strdup(attr, GFP_KERNEL_ACCOUNT);
> >  	if (!alt_ifname)
> >  		return -ENOMEM;  
> 
> this tests the existing property size. Don't you want to test the size
> with the alt_ifname - does it make the list go over 64kB?

Do you mean counting the exact length of the string?

Or that I'm counting pre-add? That's why I added:

	size += nla_total_size(ALTIFNAMSIZ);

I like coding things up as prepare (validate) + commit,
granted it doesn't exactly look pretty here so I can recode
if you prefer. But there's no bug, right? (other than maybe 
>= could have been > but whatever).

Re: [PATCH net-next 2/2] net: limit altnames to 64k total

[David Ahern]

On 3/9/22 8:37 PM, Jakub Kicinski wrote:
> On Wed, 9 Mar 2022 19:51:07 -0700 David Ahern wrote:
>> On 3/9/22 11:29 AM, Jakub Kicinski wrote:
>>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>>> index aa05e89cc47c..159c9c61e6af 100644
>>> --- a/net/core/rtnetlink.c
>>> +++ b/net/core/rtnetlink.c
>>> @@ -3652,12 +3652,23 @@ static int rtnl_alt_ifname(int cmd, struct net_device *dev, struct nlattr *attr,
>>>  			   bool *changed, struct netlink_ext_ack *extack)
>>>  {
>>>  	char *alt_ifname;
>>> +	size_t size;
>>>  	int err;
>>>  
>>>  	err = nla_validate(attr, attr->nla_len, IFLA_MAX, ifla_policy, extack);
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	if (cmd == RTM_NEWLINKPROP) {
>>> +		size = rtnl_prop_list_size(dev);
>>> +		size += nla_total_size(ALTIFNAMSIZ);
>>> +		if (size >= U16_MAX) {
>>> +			NL_SET_ERR_MSG(extack,
>>> +				       "effective property list too long");
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>>  	alt_ifname = nla_strdup(attr, GFP_KERNEL_ACCOUNT);
>>>  	if (!alt_ifname)
>>>  		return -ENOMEM;  
>>
>> this tests the existing property size. Don't you want to test the size
>> with the alt_ifname - does it make the list go over 64kB?
> 
> Do you mean counting the exact length of the string?
> 
> Or that I'm counting pre-add? That's why I added:
> 
> 	size += nla_total_size(ALTIFNAMSIZ);
> 
> I like coding things up as prepare (validate) + commit,
> granted it doesn't exactly look pretty here so I can recode
> if you prefer. But there's no bug, right? (other than maybe 
>> = could have been > but whatever).

right. It's a worst case size estimation versus taking into account the
actual space used for the name (rtnl_prop_list_size does that for each
name so this is really conservative in space use).

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 0/2] net: control the length of the altname list

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  9 Mar 2022 10:29:12 -0800 you wrote:
> Count the memory used for altnames and don't let user
> overflow the property nlattr. This was reported by George:
> https://lore.kernel.org/all/3e564baf-a1dd-122e-2882-ff143f7eb578@gmail.com/
> 
> Targeting net-next because I think we generally don't consider
> tightening "root controlled" memory accounting to be a fix.
> There's also some risk of breakage.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: account alternate interface name memory
    https://git.kernel.org/netdev/net-next/c/5d26cff5bdbe
  - [net-next,2/2] net: limit altnames to 64k total
    https://git.kernel.org/netdev/net-next/c/155fb43b70b5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html