On 6/23/2023 3:45 PM, Julia Lawall wrote:
On Fri, 23 Jun 2023, Jeffrey Hugo wrote:
On 6/23/2023 3:14 PM, Julia Lawall wrote:
Use array_size to protect against multiplication overflows.
The changes were done using the following Coccinelle semantic patch:
// <smpl>
@@
expression E1, E2;
constant C1, C2;
identifier alloc = {vmalloc,vzalloc};
@@
(
alloc(C1 * C2,...)
|
alloc(
- (E1) * (E2)
+ array_size(E1, E2)
,...)
)
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@xxxxxxxx>
---
drivers/bus/mhi/host/init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index f72fcb66f408..34a543a67068 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -759,8 +759,8 @@ static int parse_ch_cfg(struct mhi_controller
*mhi_cntrl,
* so to avoid any memory possible allocation failures, vzalloc is
* used here
*/
- mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
- sizeof(*mhi_cntrl->mhi_chan));
+ mhi_cntrl->mhi_chan = vzalloc(array_size(mhi_cntrl->max_chan,
+ sizeof(*mhi_cntrl->mhi_chan)));
if (!mhi_cntrl->mhi_chan)
return -ENOMEM;
This doesn't seem like a good fix.
If we've overflowed the multiplication, I don't think we should continue, and
the function should return an error. array_size() is going to return
SIZE_MAX, and it looks like it is possible that vzalloc() may be able to
allocate that successfully in some scenarios. However, that is going to be
less memory than parse_ch_cfg() expected to allocate, so later on I expect the
function will still corrupt memory - basically the same result as what the
unchecked overflow would do.
I'm not convinced the semantic patch is bringing value as I suspect most of
the code being patched is in the same situation.
OK, this just brings the code in line with all the calls updated by Kees's
original patch, cited in the cover letter, which were all the
calls containing a multiplication that existed at the time.
42bc47b35320 ("treewide: Use array_size() in vmalloc()")
fad953ce0b22 ("treewide: Use array_size() in vzalloc()")
Eh. I "git show fad953ce0b22" and it doesn't really tell me much. The
commit asserts that uses of vzalloc() and multiplication need
array_size(), but doesn't really explain why.
This looks like a brute force automated update with no thought and I
fear the result of this change is the conclusion that we've solved
multiplication overflow, when it doesn't look like we've really done
much. Sure, the multiplication gets capped, but can the code actually
handle that?
I should probably run the numbers, but with the relevant spec capping
the number of channels at 256, I don't think we can realistically
approach overflow, even on a 32-bit system. However, having correct
code that is inherently safe seems like a good idea and so I feel this
function has an issue. I just don't think this automated conversion
meaningfully does anything to improve the code here.
Kees, would you please chime in and educate me here? I feel like I'm
missing something important here.
-Jeff
