Re: [PATCH] modpost: prevent stack buffer overflow in do_input_entry()

[Randy Dunlap]

Hi,

On 4/27/26 1:42 PM, Hasan Basbunar wrote:
> Several functions in scripts/mod/file2alias.c build the module alias
> string by repeatedly appending into a fixed-size on-stack buffer:
> 
> 	char alias[256] = {};
> 	...
> 	sprintf(alias + strlen(alias), "%X,*", i);
> 
> In do_input_entry() this pattern is unbounded across nine bitmap
> classes (evbit/keybit/relbit/absbit/mscbit/ledbit/sndbit/ffbit/swbit).
> The keybit case alone scans bits from INPUT_DEVICE_ID_KEY_MIN_INTERESTING
> (0x71) to INPUT_DEVICE_ID_KEY_MAX (0x2ff), which is 655 iterations; if
> a MODULE_DEVICE_TABLE(input, ...) populates keybit[] densely, the
> emission reaches ~3132 bytes — overflowing the 256-byte buffer by
> about 12x. include/linux/mod_devicetable.h declares storage for the
> full bit range ("keybit[INPUT_DEVICE_ID_KEY_MAX / BITS_PER_LONG + 1]"),
> so the worst case is reachable per the ABI.
> 

do_input() is the only do_function() that accepts such a large array "arr".
Did you look at the other do_functions() and conclude that there are no other
issues?
I wonder how large an alias do_dmi_entry() could produce?

> No driver in the current tree triggers this — every in-tree user of
> INPUT_DEVICE_ID_MATCH_KEYBIT populates keybit[] very sparsely (1-3
> bits). The concern is defense-in-depth: the unbounded sprintf is a
> silent stack-corruption primitive in a host build tool, and the buffer
> size has not been revisited since this code was added in commit
> 1d8f430c15b3 ("[PATCH] Input: add modalias support", 2005).

Maybe Fixes: that?

> Reproduced under AddressSanitizer with a stand-alone harness mirroring
> the do_input loop on a fully-populated keybit:
> 
>   ==18319==ERROR: AddressSanitizer: stack-buffer-overflow
>   WRITE of size 2 at offset 288 in frame [32, 288) 'alias'
>     #6 do_input poc.c:44
> 
>   Stack-canary build:
>   Abort trap: 6  (strlen(alias)=3134, cap was 256-1)
> 
> Add a small alias_append() helper around vsnprintf with cumulative
> bookkeeping. It calls fatal() on overflow, matching the modpost style

I probably wouldn't call that cumulative accounting.
It takes strlen(current alias) each time that it's called.
(but this is just a nit :)

> for unrecoverable build conditions. do_input() takes the buffer size
> as a new parameter; do_input_entry() passes sizeof(alias) at every
> call site. This bounds every write into the on-stack buffer and turns
> the latent overflow into a clean build error if it is ever reached.
> 
> Reported-by: Hasan Basbunar <basbunarhasan@xxxxxxxxx>
> Signed-off-by: Hasan Basbunar <basbunarhasan@xxxxxxxxx>
> ---
>  scripts/mod/file2alias.c | 70 ++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 20 deletions(-)

Reviewed-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
Tested-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>

Tested means that the before & after versions of
modules.builtin.modinfo are the same.

-- 
~Randy