[GIT,PULL] s390 fixes for 6.2-rc7
Pull-request
git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.2-4
Commit Message
Hello Linus,
please pull a couple of s390 fixes.
Thanks,
Heiko
The following changes since commit 41e1992665a2701fa025a8b76970c43b4148446f:
s390: workaround invalid gcc-11 out of bounds read warning (2023-01-17 19:00:59 +0100)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.2-4
for you to fetch changes up to 7ab41c2c08a32132ba8c14624910e2fe8ce4ba4b:
s390/decompressor: specify __decompress() buf len to avoid overflow (2023-01-31 18:54:21 +0100)
----------------------------------------------------------------
s390 fixes for 6.2-rc7
- With CONFIG_VMAP_STACK enabled it is not possible to load the s390
specific diag288_wdt watchdog module. Reason is that a pointer to a
string is passed to an inline assembly; this string however is located on
the stack, while the instruction within the inline assembly expects a
physicial address. Fix this by copying the string to a kmalloc'ed buffer.
- The diag288_wdt watchdog module does not indicate that it accesses memory
from an inline assembly, which it does. Add "memory" to the clobber list
to prevent the compiler from optimizing code incorrectly away.
- Pass size of the uncompressed kernel image to __decompress() call.
Otherwise the kernel image decompressor may corrupt/overwrite an
initrd. This was reported to happen on s390 after commit 2aa14b1ab2c4
("zstd: import usptream v1.5.2").
----------------------------------------------------------------
Alexander Egorenkov (2):
watchdog: diag288_wdt: do not use stack buffers for hardware data
watchdog: diag288_wdt: fix __diag288() inline assembly
Vasily Gorbik (1):
s390/decompressor: specify __decompress() buf len to avoid overflow
arch/s390/boot/decompressor.c | 2 +-
drivers/watchdog/diag288_wdt.c | 15 ++++++++++++---
2 files changed, 13 insertions(+), 4 deletions(-)
Comments
On Thu, Feb 2, 2023 at 12:08 PM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> - With CONFIG_VMAP_STACK enabled it is not possible to load the s390
> specific diag288_wdt watchdog module. Reason is that a pointer to a
> string is passed to an inline assembly; this string however is located on
> the stack, while the instruction within the inline assembly expects a
> physicial address. Fix this by copying the string to a kmalloc'ed buffer.
Ugh. I have pulled this, but I think that fix is disgusting.
I think the kmalloc/kfree should have been done inside __diag288_vm()
itself, where it actually does that whole "virt_to_phys()" too.
Now there are three callers of __diag288_vm(), and they all do that
ugly kmalloc game with no comment about why it's needed.
If __diag288_vm() just did it itself, and had a comment right next to
the virt_to_phys() about why, that would look a lot better, I think.
That said, I don't know the code, and maybe there was some reason to
do it this way. As mentioned, I've pulled it, I just don't
particularly love the patch.
Linus
The pull request you sent on Thu, 2 Feb 2023 21:08:46 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.2-4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/addfba11b314824e3b4fb70448b339dcb21be5bf
Thank you!
On Thu, Feb 02, 2023 at 01:01:21PM -0800, Linus Torvalds wrote:
> On Thu, Feb 2, 2023 at 12:08 PM Heiko Carstens <hca@linux.ibm.com> wrote:
> >
> > - With CONFIG_VMAP_STACK enabled it is not possible to load the s390
> > specific diag288_wdt watchdog module. Reason is that a pointer to a
> > string is passed to an inline assembly; this string however is located on
> > the stack, while the instruction within the inline assembly expects a
> > physicial address. Fix this by copying the string to a kmalloc'ed buffer.
>
> Ugh. I have pulled this, but I think that fix is disgusting.
>
> I think the kmalloc/kfree should have been done inside __diag288_vm()
> itself, where it actually does that whole "virt_to_phys()" too.
>
> Now there are three callers of __diag288_vm(), and they all do that
> ugly kmalloc game with no comment about why it's needed.
>
> If __diag288_vm() just did it itself, and had a comment right next to
> the virt_to_phys() about why, that would look a lot better, I think.
>
> That said, I don't know the code, and maybe there was some reason to
> do it this way. As mentioned, I've pulled it, I just don't
> particularly love the patch.
I really should have mentioned that this is just the minimal fix so it can
be easily backported. We have five patches pending which will cleanup the
whole driver, and which will also address what you complain about.
Patches will be sent out later today to Wim, Guenter, and the watchdog
mailing list, since Guenter already rightfully complained that I bypassed
them.
@@ -80,6 +80,6 @@ void *decompress_kernel(void)
void *output = (void *)decompress_offset;
__decompress(_compressed_start, _compressed_end - _compressed_start,
- NULL, NULL, output, 0, NULL, error);
+ NULL, NULL, output, vmlinux.image_size, NULL, error);
return output;
}
@@ -86,7 +86,7 @@ static int __diag288(unsigned int func, unsigned int timeout,
"1:\n"
EX_TABLE(0b, 1b)
: "+d" (err) : "d"(__func), "d"(__timeout),
- "d"(__action), "d"(__len) : "1", "cc");
+ "d"(__action), "d"(__len) : "1", "cc", "memory");
return err;
}
@@ -268,12 +268,21 @@ static int __init diag288_init(void)
char ebc_begin[] = {
194, 197, 199, 201, 213
};
+ char *ebc_cmd;
watchdog_set_nowayout(&wdt_dev, nowayout_info);
if (MACHINE_IS_VM) {
- if (__diag288_vm(WDT_FUNC_INIT, 15,
- ebc_begin, sizeof(ebc_begin)) != 0) {
+ ebc_cmd = kmalloc(sizeof(ebc_begin), GFP_KERNEL);
+ if (!ebc_cmd) {
+ pr_err("The watchdog cannot be initialized\n");
+ return -ENOMEM;
+ }
+ memcpy(ebc_cmd, ebc_begin, sizeof(ebc_begin));
+ ret = __diag288_vm(WDT_FUNC_INIT, 15,
+ ebc_cmd, sizeof(ebc_begin));
+ kfree(ebc_cmd);
+ if (ret != 0) {
pr_err("The watchdog cannot be initialized\n");
return -EINVAL;
}