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 <[email protected]>
---
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;
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.
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.
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