| Message ID | PAXPR02MB724871DB78375AB06B5171C88B152@PAXPR02MB7248.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e5c5f3596de224422561d48eba6ece5210d967b3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sctp: prefer struct_size over open coded arithmetic | expand |
On Sat, Apr 27, 2024 at 1:23 PM Erick Archer <erick.archer@outlook.com> wrote: > > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this > structure ends in a flexible array: > > struct sctp_assoc_ids { > [...] > sctp_assoc_t gaids_assoc_id[]; > }; > > the preferred way in the kernel is to use the struct_size() helper to > do the arithmetic instead of the calculation "size + size * count" in > the kmalloc() function. > > Also, refactor the code adding the "ids_size" variable to avoid sizing > twice. > > This way, the code is more readable and safer. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > Link: https://github.com/KSPP/linux/issues/160 [2] > Signed-off-by: Erick Archer <erick.archer@outlook.com> > --- > Hi, > > The Coccinelle script used to detect this code pattern is the following: > > virtual report > > @rule1@ > type t1; > type t2; > identifier i0; > identifier i1; > identifier i2; > identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; > position p1; > @@ > > i0 = sizeof(t1) + sizeof(t2) * i1; > ... > i2 = ALLOC@p1(..., i0, ...); > > @script:python depends on report@ > p1 << rule1.p1; > @@ > > msg = "WARNING: verify allocation on line %s" % (p1[0].line) > coccilib.report.print_report(p1[0],msg) > > Regards, > Erick > --- > net/sctp/socket.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index e416b6d3d270..64196b1dce1d 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -7119,6 +7119,7 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len, > struct sctp_sock *sp = sctp_sk(sk); > struct sctp_association *asoc; > struct sctp_assoc_ids *ids; > + size_t ids_size; > u32 num = 0; > > if (sctp_style(sk, TCP)) > @@ -7131,11 +7132,11 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len, > num++; > } > > - if (len < sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num) > + ids_size = struct_size(ids, gaids_assoc_id, num); > + if (len < ids_size) > return -EINVAL; > > - len = sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num; > - > + len = ids_size; > ids = kmalloc(len, GFP_USER | __GFP_NOWARN); > if (unlikely(!ids)) > return -ENOMEM; > -- > 2.25.1 > Acked-by: Xin Long <lucien.xin@gmail.com>
On Sat, Apr 27, 2024 at 07:23:36PM +0200, Erick Archer wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this > structure ends in a flexible array: > > struct sctp_assoc_ids { __u32 gaids_number_of_ids; > sctp_assoc_t gaids_assoc_id[]; > }; This could gain __counted_by: diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index b7d91d4cf0db..836173e73401 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -1007,7 +1007,7 @@ enum sctp_sstat_state { */ struct sctp_assoc_ids { __u32 gaids_number_of_ids; - sctp_assoc_t gaids_assoc_id[]; + sctp_assoc_t gaids_assoc_id[] __counted_by(gaids_number_of_ids); }; /* > > the preferred way in the kernel is to use the struct_size() helper to > do the arithmetic instead of the calculation "size + size * count" in > the kmalloc() function. > > Also, refactor the code adding the "ids_size" variable to avoid sizing > twice. > > This way, the code is more readable and safer. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > Link: https://github.com/KSPP/linux/issues/160 [2] > Signed-off-by: Erick Archer <erick.archer@outlook.com> > --- > Hi, > > The Coccinelle script used to detect this code pattern is the following: > > virtual report > > @rule1@ > type t1; > type t2; > identifier i0; > identifier i1; > identifier i2; > identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; > position p1; > @@ > > i0 = sizeof(t1) + sizeof(t2) * i1; > ... > i2 = ALLOC@p1(..., i0, ...); > > @script:python depends on report@ > p1 << rule1.p1; > @@ > > msg = "WARNING: verify allocation on line %s" % (p1[0].line) > coccilib.report.print_report(p1[0],msg) > > Regards, > Erick > --- > net/sctp/socket.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index e416b6d3d270..64196b1dce1d 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -7119,6 +7119,7 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len, > struct sctp_sock *sp = sctp_sk(sk); > struct sctp_association *asoc; > struct sctp_assoc_ids *ids; > + size_t ids_size; > u32 num = 0; > > if (sctp_style(sk, TCP)) > @@ -7131,11 +7132,11 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len, > num++; > } > > - if (len < sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num) > + ids_size = struct_size(ids, gaids_assoc_id, num); > + if (len < ids_size) > return -EINVAL; > > - len = sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num; > - > + len = ids_size; > ids = kmalloc(len, GFP_USER | __GFP_NOWARN); > if (unlikely(!ids)) > return -ENOMEM; But yes, this looks fine to me. Reviewed-by: Kees Cook <keescook@chromium.org>
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Sat, 27 Apr 2024 19:23:36 +0200 you wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this > structure ends in a flexible array: > > struct sctp_assoc_ids { > [...] > sctp_assoc_t gaids_assoc_id[]; > }; > > [...] Here is the summary with links: - sctp: prefer struct_size over open coded arithmetic https://git.kernel.org/netdev/net-next/c/e5c5f3596de2 You are awesome, thank you!
Hi Kees and Xin, On Mon, Apr 29, 2024 at 10:45:20AM -0700, Kees Cook wrote: > On Sat, Apr 27, 2024 at 07:23:36PM +0200, Erick Archer wrote: > > This is an effort to get rid of all multiplications from allocation > > functions in order to prevent integer overflows [1][2]. > > > > As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this > > structure ends in a flexible array: > > > > struct sctp_assoc_ids { > __u32 gaids_number_of_ids; > > sctp_assoc_t gaids_assoc_id[]; > > }; > > This could gain __counted_by: > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index b7d91d4cf0db..836173e73401 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -1007,7 +1007,7 @@ enum sctp_sstat_state { > */ > struct sctp_assoc_ids { > __u32 gaids_number_of_ids; > - sctp_assoc_t gaids_assoc_id[]; > + sctp_assoc_t gaids_assoc_id[] __counted_by(gaids_number_of_ids); > }; > Since this patch has been applied to the linux-next tree, I will send an incremental one. Thanks Kees and Xin for the review. Regards, Erick
diff --git a/net/sctp/socket.c b/net/sctp/socket.c index e416b6d3d270..64196b1dce1d 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -7119,6 +7119,7 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len, struct sctp_sock *sp = sctp_sk(sk); struct sctp_association *asoc; struct sctp_assoc_ids *ids; + size_t ids_size; u32 num = 0; if (sctp_style(sk, TCP)) @@ -7131,11 +7132,11 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len, num++; } - if (len < sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num) + ids_size = struct_size(ids, gaids_assoc_id, num); + if (len < ids_size) return -EINVAL; - len = sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num; - + len = ids_size; ids = kmalloc(len, GFP_USER | __GFP_NOWARN); if (unlikely(!ids)) return -ENOMEM;
This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1][2]. As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this structure ends in a flexible array: struct sctp_assoc_ids { [...] sctp_assoc_t gaids_assoc_id[]; }; the preferred way in the kernel is to use the struct_size() helper to do the arithmetic instead of the calculation "size + size * count" in the kmalloc() function. Also, refactor the code adding the "ids_size" variable to avoid sizing twice. This way, the code is more readable and safer. This code was detected with the help of Coccinelle, and audited and modified manually. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/160 [2] Signed-off-by: Erick Archer <erick.archer@outlook.com> --- Hi, The Coccinelle script used to detect this code pattern is the following: virtual report @rule1@ type t1; type t2; identifier i0; identifier i1; identifier i2; identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; position p1; @@ i0 = sizeof(t1) + sizeof(t2) * i1; ... i2 = ALLOC@p1(..., i0, ...); @script:python depends on report@ p1 << rule1.p1; @@ msg = "WARNING: verify allocation on line %s" % (p1[0].line) coccilib.report.print_report(p1[0],msg) Regards, Erick --- net/sctp/socket.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)