[PATCH v2 00/24] use vmalloc_array and vcalloc

[PATCH v2 00/24] use vmalloc_array and vcalloc

[Julia Lawall]

The functions vmalloc_array and vcalloc were introduced in

commit a8749a35c399 ("mm: vmalloc: introduce array allocation functions")

but are not used much yet.  This series introduces uses of
these functions, to protect against multiplication overflows.

The changes were done using the following Coccinelle semantic
patch.

@initialize:ocaml@
@@

let rename alloc =
  match alloc with
    "vmalloc" -> "vmalloc_array"
  | "vzalloc" -> "vcalloc"
  | _ -> failwith "unknown"

@@
    size_t e1,e2;
    constant C1, C2;
    expression E1, E2, COUNT, x1, x2, x3;
    typedef u8;
    typedef __u8;
    type t = {u8,__u8,char,unsigned char};
    identifier alloc = {vmalloc,vzalloc};
    fresh identifier realloc = script:ocaml(alloc) { rename alloc };
@@

(
      alloc(x1*x2*x3)
|
      alloc(C1 * C2)
|
      alloc((sizeof(t)) * (COUNT), ...)
|
-     alloc((e1) * (e2))
+     realloc(e1, e2)
|
-     alloc((e1) * (COUNT))
+     realloc(COUNT, e1)
|
-     alloc((E1) * (E2))
+     realloc(E1, E2)
)

v2: This series uses vmalloc_array and vcalloc instead of
array_size.  It also leaves a multiplication of a constant by a
sizeof as is.  Two patches are thus dropped from the series.

---

 arch/x86/kernel/cpu/sgx/main.c                    |    2 +-
 drivers/accel/habanalabs/common/device.c          |    3 ++-
 drivers/accel/habanalabs/common/state_dump.c      |    7 ++++---
 drivers/bus/mhi/host/init.c                       |    2 +-
 drivers/comedi/comedi_buf.c                       |    4 ++--
 drivers/dma-buf/heaps/system_heap.c               |    2 +-
 drivers/gpu/drm/gud/gud_pipe.c                    |    2 +-
 drivers/gpu/drm/i915/gvt/gtt.c                    |    6 ++++--
 drivers/infiniband/hw/bnxt_re/qplib_res.c         |    4 ++--
 drivers/infiniband/hw/erdma/erdma_verbs.c         |    4 ++--
 drivers/infiniband/sw/siw/siw_qp.c                |    4 ++--
 drivers/infiniband/sw/siw/siw_verbs.c             |    6 +++---
 drivers/iommu/tegra-gart.c                        |    4 ++--
 drivers/net/ethernet/amd/pds_core/core.c          |    4 ++--
 drivers/net/ethernet/freescale/enetc/enetc.c      |    4 ++--
 drivers/net/ethernet/google/gve/gve_tx.c          |    2 +-
 drivers/net/ethernet/marvell/octeon_ep/octep_rx.c |    2 +-
 drivers/net/ethernet/microsoft/mana/hw_channel.c  |    2 +-
 drivers/net/ethernet/pensando/ionic/ionic_lif.c   |    4 ++--
 drivers/scsi/fnic/fnic_trace.c                    |    2 +-
 drivers/scsi/qla2xxx/qla_init.c                   |    4 ++--
 drivers/vdpa/vdpa_user/iova_domain.c              |    4 ++--
 drivers/virtio/virtio_mem.c                       |    6 +++---
 fs/btrfs/zoned.c                                  |    4 ++--
 kernel/kcov.c                                     |    2 +-
 lib/test_vmalloc.c                                |    9 +++++----
 26 files changed, 52 insertions(+), 47 deletions(-)

[PATCH v2 21/24] x86/sgx: use vmalloc_array and vcalloc

[Julia Lawall]

Use vmalloc_array and vcalloc to protect against
multiplication overflows.

The changes were done using the following Coccinelle
semantic patch:

// <smpl>
@initialize:ocaml@
@@

let rename alloc =
  match alloc with
    "vmalloc" -> "vmalloc_array"
  | "vzalloc" -> "vcalloc"
  | _ -> failwith "unknown"

@@
    size_t e1,e2;
    constant C1, C2;
    expression E1, E2, COUNT, x1, x2, x3;
    typedef u8;
    typedef __u8;
    type t = {u8,__u8,char,unsigned char};
    identifier alloc = {vmalloc,vzalloc};
    fresh identifier realloc = script:ocaml(alloc) { rename alloc };
@@

(
      alloc(x1*x2*x3)
|
      alloc(C1 * C2)
|
      alloc((sizeof(t)) * (COUNT), ...)
|
-     alloc((e1) * (e2))
+     realloc(e1, e2)
|
-     alloc((e1) * (COUNT))
+     realloc(COUNT, e1)
|
-     alloc((E1) * (E2))
+     realloc(E1, E2)
)
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>

---
v2: Use vmalloc_array and vcalloc instead of array_size.
This also leaves a multiplication of a constant by a sizeof
as is.  Two patches are thus dropped from the series.

 arch/x86/kernel/cpu/sgx/main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -628,7 +628,7 @@ static bool __init sgx_setup_epc_section
 	if (!section->virt_addr)
 		return false;
 
-	section->pages = vmalloc(nr_pages * sizeof(struct sgx_epc_page));
+	section->pages = vmalloc_array(nr_pages, sizeof(struct sgx_epc_page));
 	if (!section->pages) {
 		memunmap(section->virt_addr);
 		return false;

Re: [PATCH v2 21/24] x86/sgx: use vmalloc_array and vcalloc

[Dave Hansen]

On 6/27/23 07:43, Julia Lawall wrote:
> Use vmalloc_array and vcalloc to protect against
> multiplication overflows.
...
> diff -u -p a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -628,7 +628,7 @@ static bool __init sgx_setup_epc_section
>  	if (!section->virt_addr)
>  		return false;
>  
> -	section->pages = vmalloc(nr_pages * sizeof(struct sgx_epc_page));
> +	section->pages = vmalloc_array(nr_pages, sizeof(struct sgx_epc_page));
>  	if (!section->pages) {

I'm not sure that changelog matches the code.

'nr_pages' here is an 'unsigned long' and The sizeof()==32.  In
practice, the multiplication can be done with a shift, and the ulong is
a *LONG* way from overflowing.

I'll accept that, as a general rule, vmalloc_array() is the preferred
form.  It's totally possible that someone could copy and paste the
nr_foo*sizeof(struct bar) code over to a place where nr_foo is a more
troublesome type.

But, if that's the true motivation, could we please say that in the
changelog?  As it stands, it's a bit silly to be talking about
multiplication overflows, unless I'm missing something totally obvious.

Re: [PATCH v2 21/24] x86/sgx: use vmalloc_array and vcalloc

[Julia Lawall]

On Tue, 27 Jun 2023, Dave Hansen wrote:

> On 6/27/23 07:43, Julia Lawall wrote:
> > Use vmalloc_array and vcalloc to protect against
> > multiplication overflows.
> ...
> > diff -u -p a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -628,7 +628,7 @@ static bool __init sgx_setup_epc_section
> >  	if (!section->virt_addr)
> >  		return false;
> >
> > -	section->pages = vmalloc(nr_pages * sizeof(struct sgx_epc_page));
> > +	section->pages = vmalloc_array(nr_pages, sizeof(struct sgx_epc_page));
> >  	if (!section->pages) {
>
> I'm not sure that changelog matches the code.
>
> 'nr_pages' here is an 'unsigned long' and The sizeof()==32.  In
> practice, the multiplication can be done with a shift, and the ulong is
> a *LONG* way from overflowing.
>
> I'll accept that, as a general rule, vmalloc_array() is the preferred
> form.  It's totally possible that someone could copy and paste the
> nr_foo*sizeof(struct bar) code over to a place where nr_foo is a more
> troublesome type.
>
> But, if that's the true motivation, could we please say that in the
> changelog?  As it stands, it's a bit silly to be talking about
> multiplication overflows, unless I'm missing something totally obvious.

If it is certain that no overflow is possible, then perhaps it is fine to
drop the patch?  I didn't change cases where both arguments are constants
nor where the result of the sizeof is 1.  But I also didn't do a careful
analysis to see if an overflow is possible given the possible values
involved.

Or if it seems better to keep the change, I can also change the log
message.

julia

Re: [PATCH v2 21/24] x86/sgx: use vmalloc_array and vcalloc

[Dave Hansen]

On 6/27/23 08:01, Julia Lawall wrote:
> If it is certain that no overflow is possible, then perhaps it is fine to
> drop the patch? 

It's impossible in practice in this case because the code is 64-bit only
and uses an 'unsigned long'.  But, like I said, I can see that same
vmalloc() being copied-and-pasted or moved to a 32-bit system and
theoretically causing problems in rare scenarios.

I'd probably just drop this patch.

Re: [PATCH v2 00/24] use vmalloc_array and vcalloc

[patchwork-bot+netdevbpf]

Hello:

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

On Tue, 27 Jun 2023 16:43:15 +0200 you wrote:
> The functions vmalloc_array and vcalloc were introduced in
> 
> commit a8749a35c399 ("mm: vmalloc: introduce array allocation functions")
> 
> but are not used much yet.  This series introduces uses of
> these functions, to protect against multiplication overflows.
> 
> [...]

Here is the summary with links:
  - [v2,02/24] octeon_ep: use vmalloc_array and vcalloc
    https://git.kernel.org/netdev/net-next/c/32d462a5c3e5
  - [v2,04/24] gve: use vmalloc_array and vcalloc
    https://git.kernel.org/netdev/net-next/c/a13de901e8d5
  - [v2,09/24] pds_core: use vmalloc_array and vcalloc
    https://git.kernel.org/netdev/net-next/c/906a76cc7645
  - [v2,11/24] ionic: use vmalloc_array and vcalloc
    https://git.kernel.org/netdev/net-next/c/f712c8297e0a
  - [v2,18/24] net: enetc: use vmalloc_array and vcalloc
    https://git.kernel.org/netdev/net-next/c/fa87c54693ae
  - [v2,22/24] net: mana: use vmalloc_array and vcalloc
    https://git.kernel.org/netdev/net-next/c/e9c74f8b8a31

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

Re: [PATCH v2 00/24] use vmalloc_array and vcalloc

[Martin K. Petersen]

Julia,

> The functions vmalloc_array and vcalloc were introduced in
>
> commit a8749a35c399 ("mm: vmalloc: introduce array allocation functions")
>
> but are not used much yet.  This series introduces uses of
> these functions, to protect against multiplication overflows.

Applied #7 and #24 to 6.5/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: (subset) [PATCH v2 00/24] use vmalloc_array and vcalloc

[Martin K. Petersen]

On Tue, 27 Jun 2023 16:43:15 +0200, Julia Lawall wrote:

> The functions vmalloc_array and vcalloc were introduced in
> 
> commit a8749a35c399 ("mm: vmalloc: introduce array allocation functions")
> 
> but are not used much yet.  This series introduces uses of
> these functions, to protect against multiplication overflows.
> 
> [...]

Applied to 6.5/scsi-fixes, thanks!

[07/24] scsi: fnic: use vmalloc_array and vcalloc
        https://git.kernel.org/mkp/scsi/c/b34c7dcaf311
[24/24] scsi: qla2xxx: use vmalloc_array and vcalloc
        https://git.kernel.org/mkp/scsi/c/04d91b783acf

-- 
Martin K. Petersen	Oracle Linux Engineering