Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the
 setup_header length calculation
To:     Daniel Kiper <daniel.kiper@oracle.com>, grub-devel@gnu.org,
        linux-kernel@vger.kernel.org, x86@kernel.org
Cc:     bp@alien8.de, eric.snowberg@oracle.com, hpa@zytor.com,
        kanth.ghatraju@oracle.com, konrad.wilk@oracle.com,
        mingo@redhat.com, phcoder@gmail.com, rdunlap@infradead.org
References: <20191202172939.29271-1-daniel.kiper@oracle.com>
From:   Ross Philipson <ross.philipson@oracle.com>
Openpgp: preference=signencrypt
Autocrypt: addr=ross.philipson@oracle.com; keydata=
 xsBNBFtHZ04BCADHhtvMImplKfIEOytU7ZH4haZ9eFAqZpGeIpG9D+pzTCuReM2/49bvgNoI
 e1xuiQFO+UEJ8FjedFjDdqY7fSw3xVdX9gLwD1Rmw0Dadc1w6sGbcoOQLHcglesu+BmcKBtU
 tWQZkzCpEShN4etgZThk8469YnAvO08vNZsrizgrpD90T7mEYiNXxIkX87sPGbnBrL1X7RvZ
 TaRXfE8174W+XVwGEpSiO/GjRgLW8+DFZB5MgXpCR993+U1YT9Lz97/MRzr4hqcOYry6LBYi
 s8dOly4oP7gK15oW8Xap9+ur0Jd8Vy8o99Axq+7yunF+2KE2SwP3/w8H3VDpx7EeDhWDABEB
 AAHNKlJvc3MgUGhpbGlwc29uIDxyb3NzLnBoaWxpcHNvbkBvcmFjbGUuY29tPsLAlAQTAQgA
 PgIbAwULCQgHAgYVCgkICwIEFgIDAQIeAQIXgBYhBFsN7r6v0OZTCaJ1wdpHdTZHiMYcBQJb
 R2eBBQkJZgGzAAoJENpHdTZHiMYcPYcH/Rlp3/F3P4/2i/W0F4yQDVD6rAkejCws4KlbgC5D
 Slkdvk6j8jOW/HNeIY3n+a3mW0iyyhZlipgYAqkK1loDiDxJjc2eUaHxiYWNLQ4CwIj2EC27
 AWCp6hgwHNWmZrdeNbM/Z6LTFQILx5xzgX+86KNqzFV7gOcAaS2qBVz1D83dgrFZaGaao918
 nvfe+SnImo0GaEf8nVDKgsD2zfzMBkk4q/E0mrEADFXwBHSvNCnVyrCN6Ve0dHWgI7SszUDt
 7v01zbGPR5mRfGuyC9gykd2SDCw5/Q27RMWfaPFL/dtiZBljUzb2yW5jicZAz7zNdDcBSUGR
 r//wxtG4k/dBrMXOwE0EW0dnTwEIAPelEnLDnfJnHdFR+1Thrvv3Udt/1cjqQfHqH4F8zef/
 MsIcPV1skL7qPUYD+CrbasvmqhlPxtJAtN68inPa70fA2g0PtNmLUH1NBb2e6EjOoVZg9ais
 BWfdYUITZouOXs2zCTFsoNWjTJANnXxexbTf1ZEqfzlVtQK+xAnXl3kiL4Y47VMbgDkGedhw
 3ZMWQ2zMMZqYJkPYhtlTXtedhV91DL1347ULwHsvkUJDZ0gL+WU6tYhsCOOiD61x58PfUiFb
 /WkZEPxb96dSSSWrTlLlBWSSD24RnhfbJjfsXeSu9s4XldmGTDkj7jclMVU1xV0BUfqEwhVn
 xR8FlC+dZvkAEQEAAcLAfAQYAQgAJgIbDBYhBFsN7r6v0OZTCaJ1wdpHdTZHiMYcBQJbR2eB
 BQkJZgGyAAoJENpHdTZHiMYcDIAIAIRJrKjIStRvLsOOCX92s9XJPUjrC/xmtVsqVviyFWIC
 QRPQzDE+bDSvRazudBHmcPW+BOOB5B+p7zKZzOGoZV2peG8oA/Y8oCxOYBtpbBaZ5KJexm/g
 BbnJUwb3uhmKtDShHGUCmtq8MZBJBr6Q6xHprOU8Qnzs9Jea8NVwaz9O226Rrg4XVv/sK1Lh
 ++xZfhi7YqKWdx5vdfdnX1xWe8ma0eXLeCDh3V6Ys+Habw1jEbMuafrcVzAbp1rMt2Lju1ls
 BNAoxeViK7QXWfwGTmGJP++jHmo99gMqEtiohf+37N0oS6uYu6kaE7PxsEcOjWKJxW/DdgwO
 eFq+D6xuiKk=
Message-ID: <7c512bb8-8c55-803d-3eee-2198201c56cb@oracle.com>
Date:   Fri, 13 Dec 2019 13:43:13 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20191202172939.29271-1-daniel.kiper@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 12/02/2019 12:29 PM, Daniel Kiper wrote:
> Recent work around x86 Linux kernel loader revealed an underflow in the
> setup_header length calculation and another related issue. Both lead to
> the memory overwrite and later machine crash.
> 
> Currently when the GRUB copies the setup_header into the linux_params
> (struct boot_params, traditionally known as "zero page") it assumes the
> setup_header size as sizeof(linux_i386_kernel_header/lh). This is
> incorrect. It should use the value calculated accordingly to the Linux
> kernel boot protocol. Otherwise in case of pretty old kernel, to be
> exact Linux kernel boot protocol, the GRUB may write more into
> linux_params than it was expected to. Fortunately this is not very big
> issue. Though it has to be fixed. However, there is also an underflow
> which is grave. It happens when
> 
>   sizeof(linux_i386_kernel_header/lh) > "real size of the setup_header".
> 
> Then len value wraps around and grub_file_read() reads whole kernel into
> the linux_params overwriting memory past it. This leads to the GRUB
> memory allocator breakage and finally to its crash during boot.
> 
> The patch fixes both issues. Additionally, it moves the code not related to
> grub_memset(linux_params)/grub_memcpy(linux_params)/grub_file_read(linux_params)
> section outside of it to not confuse the reader.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

Yes this looks correct. The length should be based off the jmp offset
byte and not the size of the structure defined in GRUB.

Reviewed-by: Ross Philipson <ross.philipson@oracle.com>

> ---
>  grub-core/loader/i386/linux.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index d0501e229..ee95cd374 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -761,17 +761,12 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>      goto fail;
>  
>    grub_memset (&linux_params, 0, sizeof (linux_params));
> -  grub_memcpy (&linux_params.setup_sects, &lh.setup_sects, sizeof (lh) - 0x1F1);
> -
> -  linux_params.code32_start = prot_mode_target + lh.code32_start - GRUB_LINUX_BZIMAGE_ADDR;
> -  linux_params.kernel_alignment = (1 << align);
> -  linux_params.ps_mouse = linux_params.padding10 =  0;
>  
>    /*
>     * The Linux 32-bit boot protocol defines the setup header end
>     * to be at 0x202 + the byte value at 0x201.
>     */
> -  len = 0x202 + *((char *) &linux_params.jump + 1);
> +  len = 0x202 + *((char *) &lh.jump + 1);
>  
>    /* Verify the struct is big enough so we do not write past the end. */
>    if (len > (char *) &linux_params.edd_mbr_sig_buffer - (char *) &linux_params) {
> @@ -779,10 +774,13 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>      goto fail;
>    }
>  
> +  grub_memcpy (&linux_params.setup_sects, &lh.setup_sects, len - 0x1F1);
> +
>    /* We've already read lh so there is no need to read it second time. */
>    len -= sizeof(lh);
>  
> -  if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len)
> +  if ((len > 0) &&
> +      (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len))
>      {
>        if (!grub_errno)
>  	grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
> @@ -790,6 +788,9 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>        goto fail;
>      }
>  
> +  linux_params.code32_start = prot_mode_target + lh.code32_start - GRUB_LINUX_BZIMAGE_ADDR;
> +  linux_params.kernel_alignment = (1 << align);
> +  linux_params.ps_mouse = linux_params.padding10 = 0;
>    linux_params.type_of_loader = GRUB_LINUX_BOOT_LOADER_TYPE;
>  
>    /* These two are used (instead of cmd_line_ptr) by older versions of Linux,
>