Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Dec 2022 11:13:32 -0800
From:   "H. Peter Anvin" <hpa@zytor.com>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
CC:     pbonzini@redhat.com, ebiggers@kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, qemu-devel@nongnu.org,
        ardb@kernel.org, kraxel@redhat.com, bp@alien8.de, philmd@linaro.org
Subject: =?US-ASCII?Q?Re=3A_=5BPATCH_qemu=5D_x86=3A_don=27t_let_decomp?= =?US-ASCII?Q?ressed_kernel_image_clobber_setup=5Fdata?=
User-Agent: K-9 Mail for Android
In-Reply-To: <Y68K4mPuz6edQkCX@zx2c4.com>
References: <20221228143831.396245-1-Jason@zx2c4.com> <6cab26b5-06ae-468d-ac79-ecdecb86ef07@linaro.org> <Y6xvJheSYC83voCZ@zx2c4.com> <Y6x1knb8udpSyMSp@zx2c4.com> <9188EEE9-2759-4389-B39E-0FEBBA3FA57D@zytor.com> <Y6z765zHrQ6Rl/0o@zx2c4.com> <AF921575-0968-434A-8B46-095B78C209C1@zytor.com> <Y68K4mPuz6edQkCX@zx2c4.com>
Message-ID: <6C1D0560-6D77-4733-9B8D-5184935AEC62@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On December 30, 2022 7:59:30 AM PST, "Jason A=2E Donenfeld" <Jason@zx2c4=2E=
com> wrote:
>Hi,
>
>On Wed, Dec 28, 2022 at 11:31:34PM -0800, H=2E Peter Anvin wrote:
>> On December 28, 2022 6:31:07 PM PST, "Jason A=2E Donenfeld" <Jason@zx2c=
4=2Ecom> wrote:
>> >Hi,
>> >
>> >Read this message in a fixed width text editor with a lot of columns=
=2E
>> >
>> >On Wed, Dec 28, 2022 at 03:58:12PM -0800, H=2E Peter Anvin wrote:
>> >> Glad you asked=2E
>> >>=20
>> >> So the kernel load addresses are parameterized in the kernel image
>> >> setup header=2E One of the things that are so parameterized are the =
size
>> >> and possible realignment of the kernel image in memory=2E
>> >>=20
>> >> I'm very confused where you are getting the 64 MB number from=2E The=
re
>> >> should not be any such limitation=2E
>> >
>> >Currently, QEMU appends it to the kernel image, not to the initramfs a=
s
>> >you suggest below=2E So, that winds up looking, currently, like:
>> >
>> >          kernel image            setup_data
>> >   |--------------------------||----------------|
>> >0x100000                  0x100000+l1     0x100000+l1+l2
>> >
>> >The problem is that this decompresses to 0x1000000 (one more zero)=2E =
So
>> >if l1 is > (0x1000000-0x100000), then this winds up looking like:
>> >
>> >          kernel image            setup_data
>> >   |--------------------------||----------------|
>> >0x100000                  0x100000+l1     0x100000+l1+l2
>> >
>> >                                 d e c o m p r e s s e d   k e r n e l
>> >		     |-------------------------------------------------------------|
>> >                0x1000000                                             =
        0x1000000+l3=20
>> >
>> >The decompressed kernel seemingly overwriting the compressed kernel
>> >image isn't a problem, because that gets relocated to a higher address
>> >early on in the boot process=2E setup_data, however, stays in the same
>> >place, since those links are self referential and nothing fixes them u=
p=2E
>> >So the decompressed kernel clobbers it=2E
>> >
>> >The solution in this commit adds a bunch of padding between the kernel
>> >image and setup_data to avoid this=2E That looks like this:
>> >
>> >          kernel image                            padding             =
                  setup_data
>> >   |--------------------------||--------------------------------------=
-------------||----------------|
>> >0x100000                  0x100000+l1                                 =
        0x1000000+l3      0x1000000+l3+l2
>> >
>> >                                 d e c o m p r e s s e d   k e r n e l
>> >		     |-------------------------------------------------------------|
>> >                0x1000000                                             =
        0x1000000+l3=20
>> >
>> >This way, the decompressed kernel doesn't clobber setup_data=2E
>> >
>> >The problem is that if 0x1000000+l3-0x100000 is around 62 megabytes,
>> >then the bootloader crashes when trying to dereference setup_data's
>> >->len param at the end of initialize_identity_maps() in ident_map_64=
=2Ec=2E
>> >I don't know why it does this=2E If I could remove the 62 megabyte
>> >restriction, then I could keep with this technique and all would be
>> >well=2E
>> >
>> >> In general, setup_data should be able to go anywhere the initrd can
>> >> go, and so is subject to the same address cap (896 MB for old kernel=
s,
>> >> 4 GB on newer ones; this address too is enumerated in the header=2E)
>> >
>> >It would be theoretically possible to attach it to the initrd image
>> >instead of to the kernel image=2E As a last resort, I guess I can look
>> >into doing that=2E However, that's going to require some serious rewor=
k
>> >and plumbing of a lot of different components=2E So if I can make it w=
ork
>> >as is, that'd be ideal=2E However, I need to figure out this weird 62 =
meg
>> >limitation=2E
>> >
>> >Any ideas on that?
>> >
>> >Jason
>>=20
>> As far as a crash=2E=2E=2E that sounds like a big and a pretty serious =
one at that=2E
>>=20
>> Could you let me know what kernel you are using and how *exactly* you a=
re booting it?
>
>I'll attach a =2Econfig file=2E Apply the patch at the top of this thread=
 to
>qemu, except make one modification:
>
>diff --git a/hw/i386/x86=2Ec b/hw/i386/x86=2Ec
>index 628fd2b2e9=2E=2Ea61ee23e13 100644
>--- a/hw/i386/x86=2Ec
>+++ b/hw/i386/x86=2Ec
>@@ -1097,7 +1097,7 @@ void x86_load_linux(X86MachineState *x86ms,
>=20
>             /* The early stage can't address past around 64 MB from the =
original
>              * mapping, so just give up in that case=2E */
>-            if (padded_size < 62 * 1024 * 1024)
>+            if (true || padded_size < 62 * 1024 * 1024)
>                 kernel_size =3D padded_size;
>             else {
>                 fprintf(stderr, "qemu: Kernel image too large to hold se=
tup_data\n");
>
>Then build qemu=2E Run it with `-kernel bzImage`, based on the kernel
>built with the =2Econfig I attached=2E
>
>You'll see that the CPU triple faults when hitting this line:
>
>        sd =3D (struct setup_data *)boot_params->hdr=2Esetup_data;
>        while (sd) {
>                unsigned long sd_addr =3D (unsigned long)sd;
>
>                kernel_add_identity_map(sd_addr, sd_addr + sizeof(*sd) + =
sd->len);  <----
>                sd =3D (struct setup_data *)sd->next;
>        }
>
>, because it dereferences *sd=2E This does not happen if the decompressed
>size of the kernel is < 62 megs=2E
>
>So that's the "big and pretty serious" bug that might be worthy of
>investigation=2E
>
>Jason

No kidding=2E Dereferencing data *before you map it* is generally frowned =
upon=2E

This needs to be split into to making calls=2E

*Facepalm*