Skip to content

Backport patch to ensure NUL-terminated task->comm buffer - #168

Merged
arnaldo2792 merged 2 commits into
bottlerocket-os:developfrom
arnaldo2792:prevent-copy-non-terminated
May 30, 2025
Merged

Backport patch to ensure NUL-terminated task->comm buffer#168
arnaldo2792 merged 2 commits into
bottlerocket-os:developfrom
arnaldo2792:prevent-copy-non-terminated

Conversation

@arnaldo2792

Copy link
Copy Markdown
Contributor

Issue number:

Fixes #167

Description of changes:

Even though the kernel enforces NUL terminated strings, it seems like sometimes they do appear in the kernel causing it to panic. This has been observed upstream, particularly when reading the comm property of a task. There is a patch upstream that addresses this problem, but it doesn't apply cleanly, so back port the parts that are of interest to the strscpy function.

Testing done:

  • I built an AMI with the patch, and confirmed the instance boots. Plus, we have anecdotal data that suggests the panic doesn't occur after this patch is applied.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@bcressey bcressey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be submitted upstream, let's figure out how to do that.

Comment thread packages/kernel-6.1/kernel-6.1.spec Outdated
# Drop AL revert of upstream patch to minimize delta. The necessary dependency
# options for nvidia are instead included through DRM_SIMPLE
Patch1005: 1005-Revert-Revert-drm-fb_helper-improve-CONFIG_FB-depend.patch
# Prevent copying possible non-terminated strings

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the patch has wider ramifications because of the many uses of strscpy, the specific motivation is to ensure NUL-termination for task->comm:

Suggested change
# Prevent copying possible non-terminated strings
# Backport patch to ensure NUL-terminated task->comm buffer

Backport patch to prevent copying non-terminated strings through strscpy

Signed-off-by: Arnaldo Garcia Rincon <agarrcia@amazon.com>
@arnaldo2792
arnaldo2792 force-pushed the prevent-copy-non-terminated branch from 8fc5415 to 7dbde8c Compare May 29, 2025 21:32
@arnaldo2792

Copy link
Copy Markdown
Contributor Author

Forced push includes:

  • Address feedback in 6.1 spec file
  • Backport the original commit for the 6.12 kernel

@arnaldo2792
arnaldo2792 requested review from bcressey and larvacea May 29, 2025 21:34
@arnaldo2792 arnaldo2792 changed the title Backport patch to prevent copying non-terminated strings Backport patch to ensure NUL-terminated task->comm buffer May 29, 2025
Comment on lines +57 to +60

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention for this kind of backport appears to be:

commit <xyz> upstream.

<original commit message>

<original sign-off chain>
<your sign-off>

See this commit for an example.

So the patch would look like:

commit 9022ed0e7e65734d83a0648648589b9fbea8e8c9 upstream.

The point behind strscpy() was to once and for all avoid all the
problems with 'strncpy()' and later broken "fixed" versions like
strlcpy() that just made things worse.
...

So avoid any "copy possibly non-terminated string, and terminate later"
behavior, and write the destination buffer only once.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Arnaldo Garcia Rincon <agarrcia@amazon.com>

For the 6.1 patch, you could do the same thing but also add Shikha's sign-off.

This commit gives an example of how a [ note describing backport changes.] can be formatted. If present (as it would be for the 6.1 patch) it should come before the new sign-offs.

@@ -0,0 +1,119 @@
From b7d33b4621b8e722f69df30443421712c706bf6c Mon Sep 17 00:00:00 2001

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
From b7d33b4621b8e722f69df30443421712c706bf6c Mon Sep 17 00:00:00 2001
From 9022ed0e7e65734d83a0648648589b9fbea8e8c9 Mon Sep 17 00:00:00 2001

@arnaldo2792
arnaldo2792 force-pushed the prevent-copy-non-terminated branch from 7dbde8c to 471c66e Compare May 29, 2025 23:06
@arnaldo2792

Copy link
Copy Markdown
Contributor Author

Forced push addresses:

  • Commit ID matches the upstream commit
  • The patch is formatted as suggested

Backport patch to prevent copying non-terminated strings through strscpy

Signed-off-by: Arnaldo Garcia Rincon <agarrcia@amazon.com>
@arnaldo2792
arnaldo2792 force-pushed the prevent-copy-non-terminated branch from 471c66e to 6bfc47a Compare May 29, 2025 23:52
@arnaldo2792

Copy link
Copy Markdown
Contributor Author

(Forced push fixes the patch format for the 6.1 kernel per suggestions)

@arnaldo2792
arnaldo2792 requested a review from sky1122 May 30, 2025 00:20
@arnaldo2792
arnaldo2792 merged commit ff8bda8 into bottlerocket-os:develop May 30, 2025
Comment on lines +61 to +64
[vyaghras:
- backport to 6.1
- apply changes in original patch to strscpy, since sized_strscpy
doesn't exist in 6.1]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the example commit I linked to, and my earlier offline suggestion, it might be more conventional to write this note like this:

[vyaghras: adjust context since e6584c3964f2f ("string: Allow 2-argument
strscpy()") is not present]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kernel panic caused by potential non-terminated string when using cgroupsv1 and perf tracing

4 participants