Re: [PATCH] s390: Warn if kernel command line contains non-printable EBCDIC characters

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer underflow in size calculation leads to out-of-bounds write when appending IPL block parameters to a maximum-length kernel command line.
--

--- Patch [1]: [PATCH] s390: Warn if kernel command line contains non-printable EBCDIC characters ---
commit 0d3635825635bf3ebe41f7b2b8024aab279b4f38
Author: Ilya Leoshkevich <[email protected]>

s390: Warn if kernel command line contains non-printable EBCDIC characters

This commit adds a warning if the kernel command line contains non-printable
characters after being converted from EBCDIC to ASCII. This helps detect
accidentally pasted multi-byte UTF-8 sequences in zipl.conf that would
otherwise render the entire command line unusable.

> diff --git a/arch/s390/boot/ipl_parm.c b/arch/s390/boot/ipl_parm.c
> index 6bc950b92be76..71c0c26d56bba 100644
> --- a/arch/s390/boot/ipl_parm.c
> +++ b/arch/s390/boot/ipl_parm.c

[ ... ]

> @@ -173,12 +173,29 @@ static inline int has_ebcdic_char(const char *str)
>  void setup_boot_command_line(void)
>  {
>  	parmarea.command_line[COMMAND_LINE_SIZE - 1] = 0;
>  	/* convert arch command line to ascii if necessary */
> -	if (has_ebcdic_char(parmarea.command_line))
> +	if (has_ebcdic_char(parmarea.command_line)) {
>  		EBCASC(parmarea.command_line, COMMAND_LINE_SIZE);
> +		if (has_nonprintable_char(parmarea.command_line))
> +			boot_warn("Kernel command line was treated as EBCDIC, but contains non-printable characters\n");
> +	}
>  	/* copy arch command line */
>  	strscpy(early_command_line, strim(parmarea.command_line));
>  

[Severity: High]
This is a pre-existing issue, but since we are dealing with the bounds of
the command line buffer here, could a maximally long command line trigger
a buffer overflow shortly after this point?

Right after the quoted code, setup_boot_command_line() calls
append_ipl_block_parm(), which calculates the remaining space in the buffer:

arch/s390/boot/ipl_parm.c:append_ipl_block_parm() {
    ...
    len = strlen(early_command_line);

    delim = early_command_line + len;    /* '\0' character position */
    parm = early_command_line + len + 1; /* append right after '\0' */
    ...
    rc = ipl_block_get_ascii_vmparm(
        parm, COMMAND_LINE_SIZE - len - 1, &ipl_block);
    ...
}

If early_command_line is filled to its maximum capacity (COMMAND_LINE_SIZE - 1),
the remaining size passed to ipl_block_get_ascii_vmparm() evaluates to 0.

Inside ipl_block_get_ascii_vmparm(), the size calculation underflows:

arch/s390/kernel/ipl_vmparm.c:ipl_block_get_ascii_vmparm() {
    ...
    len = min_t(size_t, size - 1, ipb->ccw.vm_parm_len);
    memcpy(dest, ipb->ccw.vm_parm, len);
}

Because size is 0 and size_t is unsigned, size - 1 underflows to SIZE_MAX.
This bypasses the bounds check and allows the memcpy() to write out of bounds.

The same underflow pattern appears in ipl_block_get_ascii_scpdata():

arch/s390/boot/ipl_parm.c:ipl_block_get_ascii_scpdata() {
    ...
    count = min(size - 1, scpdata_length(scp_data, scp_data_len));
    ...
    memcpy(dest, scp_data, count);
}

Should there be a check ensuring size > 0 before calling these functions,
or a safeguard within the get_ascii functions to prevent the underflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1