net/ipv4: Replace one-element array with flexible-array member
Subsystems (3)
PATCH 1/1 net/ipv4: Replace one-element array with flexible-array member
There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2].
Use an anonymous union with a couple of anonymous structs in order to keep userspace unchanged:
$ pahole -C ip_msfilter net/ipv4/ip_sockglue.o
struct ip_msfilter {
union {
struct {
__be32 imsf_multiaddr_aux; /* 0 4 */
__be32 imsf_interface_aux; /* 4 4 */
__u32 imsf_fmode_aux; /* 8 4 */
__u32 imsf_numsrc_aux; /* 12 4 */
__be32 imsf_slist[1]; /* 16 4 */
}; /* 0 20 */
struct {
__be32 imsf_multiaddr; /* 0 4 */
__be32 imsf_interface; /* 4 4 */
__u32 imsf_fmode; /* 8 4 */
__u32 imsf_numsrc; /* 12 4 */
__be32 imsf_slist_flex[0]; /* 16 0 */
}; /* 0 16 */
}; /* 0 20 *//* size: 20, cachelines: 1, members: 1 */ /* last cacheline: 20 bytes */ };
Also, refactor the code accordingly and make use of the struct_size() and flex_array_size() helpers.
This helps with the ongoing efforts to globally enable -Warray-bounds and get us closer to being able to tighten the FORTIFY_SOURCE routines on memcpy().
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
--- include/uapi/linux/in.h | 21 ++++++++++++++++----- net/ipv4/igmp.c | 12 ++++++------ net/ipv4/ip_sockglue.c | 15 ++++++++------- 3 files changed, 30 insertions(+), 18 deletions(-)
include/uapi/linux/in.h +16 -5
@@ -188,11 +188,22 @@ struct ip_mreq_source { };struct ip_msfilter { -__be32 imsf_multiaddr; -__be32 imsf_interface; -__u32 imsf_fmode; -__u32 imsf_numsrc; -__be32 imsf_slist[1 ]; +union { +struct { +__be32 imsf_multiaddr_aux; +__be32 imsf_interface_aux; +__u32 imsf_fmode_aux; +__u32 imsf_numsrc_aux; +__be32 imsf_slist[1 ]; + }; +struct { +__be32 imsf_multiaddr; +__be32 imsf_interface; +__u32 imsf_fmode; +__u32 imsf_numsrc; +__be32 imsf_slist_flex[]; + }; + }; };#define IP_MSFILTER_SIZE (numsrc) \
net/ipv4/igmp.c +6 -6
@@ -2475,8 +2475,8 @@ int ip_mc_msfilter(struct sock *sk, struct ip_msfilter *msf, int ifindex) goto done; } newpsl->sl_max = newpsl->sl_count = msf->imsf_numsrc; -memcpy (newpsl->sl_addr, msf->imsf_slist, - msf->imsf_numsrc *sizeof (msf->imsf_slist[0 ])); +memcpy (newpsl->sl_addr, msf->imsf_slist_flex, +flex_array_size (msf, imsf_slist_flex, msf->imsf_numsrc)); err =ip_mc_add_src (in_dev, &msf->imsf_multiaddr, msf->imsf_fmode, newpsl->sl_count, newpsl->sl_addr,0 );if (err) {
@@ -2551,14 +2551,14 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf, count = psl->sl_count; } copycount = count < msf->imsf_numsrc ? count : msf->imsf_numsrc; - len = copycount *sizeof (psl->sl_addr[0 ]); + len =flex_array_size (psl, sl_addr, copycount); msf->imsf_numsrc = count; -if (put_user (IP_MSFILTER_SIZE (copycount), optlen) || -copy_to_user (optval, msf,IP_MSFILTER_SIZE (0 ))) { +if (put_user (struct_size (optval, imsf_slist_flex, copycount), optlen) || +copy_to_user (optval, msf,struct_size (optval, imsf_slist_flex,0 ))) {return -EFAULT ; }if (len && -copy_to_user (&optval->imsf_slist[0 ], psl->sl_addr, len)) +copy_to_user (&optval->imsf_slist_flex[0 ], psl->sl_addr, len))return -EFAULT ;return 0 ; done:
net/ipv4/ip_sockglue.c +8 -7
@@ -663,12 +663,11 @@ static int set_mcast_msfilter(struct sock *sk, int ifindex,struct sockaddr_storage *group, struct sockaddr_storage *list) { - int msize = IP_MSFILTER_SIZE(numsrc); struct ip_msfilter *msf;struct sockaddr_in *psin;int err, i; - msf =kmalloc (msize,GFP_KERNEL ); + msf =kmalloc (struct_size (msf, imsf_slist_flex, numsrc),GFP_KERNEL );if (!msf)return -ENOBUFS ;
@@ -684,7 +683,7 @@ static int set_mcast_msfilter(struct sock *sk, int ifindex,if (psin->sin_family !=AF_INET ) goto Eaddrnotavail; - msf->imsf_slist[i] = psin->sin_addr.s_addr; + msf->imsf_slist_flex[i] = psin->sin_addr.s_addr; } err =ip_mc_msfilter (sk, msf, ifindex);kfree (msf);
@@ -1229,7 +1228,7 @@ static int do_ip_setsockopt(struct sock *sk, int level, int optname, {struct ip_msfilter *msf; -if (optlen <IP_MSFILTER_SIZE (0 )) +if (optlen <struct_size (msf, imsf_slist_flex,0 )) goto e_inval;if (optlen > sysctl_optmem_max) { err = -ENOBUFS ;
@@ -1247,7 +1246,8 @@ static int do_ip_setsockopt(struct sock *sk, int level, int optname, err = -ENOBUFS ;break ; } -if (IP_MSFILTER_SIZE (msf->imsf_numsrc) > optlen) { +if (struct_size (msf, imsf_slist_flex, msf->imsf_numsrc) > + optlen) {kfree (msf); err = -EINVAL ;break ;
@@ -1660,11 +1660,12 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname, {struct ip_msfilter msf; -if (len <IP_MSFILTER_SIZE (0 )) { +if (len <struct_size (&msf, imsf_slist_flex,0 )) { err = -EINVAL ; goto out; } -if (copy_from_user (&msf, optval,IP_MSFILTER_SIZE (0 ))) { +if (copy_from_user (&msf, optval, +struct_size (&msf, imsf_slist_flex,0 ))) { err = -EFAULT ; goto out; }
Signature · 1 line
2.27.0
A message that is not in the archive (sent off-list or not yet received): 162791400741.18419.5941105433257893840.git-patchwork-notify@kernel.org
Quoted 10 lines · On 8/2/21 09:20, [email protected] wrote:
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
Quoted 5 lines · On Sat, 31 Jul 2021 12:08:30 -0500 you wrote:
There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2].
Quoted 8 lines
Quoted 4 lines
Use an anonymous union with a couple of anonymous structs in order to keep userspace unchanged:
[...]
Here is the summary with links:
- [next] net/ipv4: Replace one-element array with flexible-array memberhttps://git.kernel.org/netdev/net-next/c/2d3e5caf96b9
arghh... this has a bug. Sorry, Dave. I will send a fix for this, shortly.
--
Gustavo
Quoted 16 lines · Gustavo A. R. Silva
Quoted 13 lines · On 8/2/21 09:20, [email protected] wrote:
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
Quoted 8 lines · On Sat, 31 Jul 2021 12:08:30 -0500 you wrote:
There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2].
Use an anonymous union with a couple of anonymous structs in order to keep userspace unchanged:
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
[...]
Here is the summary with links:
- [next] net/ipv4: Replace one-element array with flexible-array memberhttps://git.kernel.org/netdev/net-next/c/2d3e5caf96b9arghh... this has a bug. Sorry, Dave. I will send a fix for this, shortly.
BTW... can we expect msf->imsf_numsrc to be zero at some point in the following pieces of code?
net/ipv4/igmp.c:
2553 copycount = count < msf->imsf_numsrc ? count : msf->imsf_numsrc; 2554 len = copycount * sizeof(psl->sl_addr[0]); 2555 msf->imsf_numsrc = count; 2556 if (put_user(IP_MSFILTER_SIZE(copycount), optlen) ||
2557 copy_to_user(optval, msf, IP_MSFILTER_SIZE(0))) {
2558 return -EFAULT;
2559 }
net/ipv4/ip_sockglue.c:
1228 case IP_MSFILTER:
1229 {
1230 struct ip_msfilter *msf;
1231
1232 if (optlen < IP_MSFILTER_SIZE(0))
1233 goto e_inval;
1234 if (optlen > sysctl_optmem_max) {
1235 err = -ENOBUFS;
1236 break;
1237 }
1238 msf = memdup_sockptr(optval, optlen);
1239 if (IS_ERR(msf)) {...
1250 if (IP_MSFILTER_SIZE(msf->imsf_numsrc) > optlen) {
1251 kfree(msf); 1252 err = -EINVAL; 1253 break; 1254 } 1255 err = ip_mc_msfilter(sk, msf, 0); 1256 kfree(msf); 1257 break;
1258 }
Thanks
--
Gustavo