I'm just looking through the kernel for krealloc() abuse, and the 'interesting' code in mvebu_pinctrl_build_functions() came to my attention.
First it allocates an array 'funcs' as follows: /* we allocate functions for number of pins and hope * there are less unique functions than pins available */ funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); (note: s/less/fewer/ in the comment). Then it populates the array, seemly trusting to its "hope" and just going off the end of the array if there are more unique functions than pins? And then finally it reallocates the array to the correct size: /* with the number of unique functions and it's groups known, reallocate functions and assign group names */ funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); (note: s/it's/its/) So... if krealloc fails, we *leak* the originally-allocated 'funcs'. Except actually we don't because it was allocated with devm_kzalloc() so that's OK. If krealloc *succeeds* then I think we should get a double-free because it doesn't free the old 'funcs' via devm_kfree() so when the device is eventually released, won't devres_release_all() free it again? I'm not entirely sure what that krealloc is *for*, anyway. Apart from retrospectively reallocating the array to the size that we've already *scribbled* over? Some kind of request for forgiveness, perhaps? We should probably make the standard kfree() (and hence krealloc()) actually fail when handed pointers which were allocated with devm_kzalloc()? This completely untested patch attempts to fix it by counting the maximum number of functions in advance, then allocating the array at that size. In practice it may overallocate if there are name collisions and _add_function() returns -EEXIST. Is that something we really need to lose sleep over? diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c index c689c04..5f9ece6 100644 --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c @@ -500,13 +500,25 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev, int num = 0; int n, s; - /* we allocate functions for number of pins and hope - * there are less unique functions than pins available */ - funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins * + /* Count the number of functions prior to allocating 'funcs' array */ + for (n = 0; n < pctl->num_groups; n++) { + struct mvebu_pinctrl_group *grp = &pctl->groups[n]; + for (s = 0; s < grp->num_settings; s++) { + /* skip unsupported settings on this variant */ + if (pctl->variant && + !(pctl->variant & grp->settings[s].variant)) + continue; + + num++; + } + } + + funcs = devm_kzalloc(&pdev->dev, num * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); if (!funcs) return -ENOMEM; + num = 0; for (n = 0; n < pctl->num_groups; n++) { struct mvebu_pinctrl_group *grp = &pctl->groups[n]; for (s = 0; s < grp->num_settings; s++) { @@ -523,13 +535,6 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev, } } - /* with the number of unique functions and it's groups known, - reallocate functions and assign group names */ - funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function), - GFP_KERNEL); - if (!funcs) - return -ENOMEM; - pctl->num_functions = num; pctl->functions = funcs; -- dwmw2
smime.p7s
Description: S/MIME cryptographic signature