Skip to content

Conversation

PlaidCat
Copy link
Collaborator

Basically the 8.10 version of this PR: #548

    media: uvcvideo: Remove dangling pointers

    jira VULN-53462
    jira VULN-53461
    cve CVE-2024-58002
    commit-author Ricardo Ribalda <ribalda@chromium.org>
    commit 221cd51efe4565501a3dbf04cc011b537dcce7fb
    upstream-diff used kernel-lt 5.15 commit 117f7a2975baa4b7d702d3f4830d5a4ebd0c6d50
            This is due to missing both:
             - e8c07082a810 - Kbuild: move to -std=gnu11
             - 54da6a092431 - locking: Introduce __cleanup() based infrastructure
    media: uvcvideo: Only save async fh if success

    jira VULN-53462
    jira VULN-53461
    cve-pre CVE-2024-58002
    commit-author Ricardo Ribalda <ribalda@chromium.org>
    commit d9fecd096f67a4469536e040a8a10bbfb665918b
    media: uvcvideo: Refactor iterators

    jira VULN-53462
    jira VULN-53461
    cve-pre CVE-2024-58002
    commit-author Ricardo Ribalda <ribalda@chromium.org>
    commit 64627daf0c5f7838111f52bbbd1a597cb5d6871a

BUILD

[jmaple@devbox code]$ egrep -B 5 -A 5 "\[TIMER\]|^Starting Build" $(ls -t kbuild* | head -n1)
/mnt/code/kernel-src-tree-build
Running make mrproper...
  CLEAN   cscope.in.out cscope.po.out cscope.out cscope.files
[TIMER]{MRPROPER}: 5s
x86_64 architecture detected, copying config
'configs/kernel-x86_64.config' -> '.config'
Setting Local Version for build
CONFIG_LOCALVERSION="-jmaple_fips-8-compliant_4.18.0-553.16.1-f51d42ab35ac"
Making olddefconfig
--
  HOSTLD  scripts/kconfig/conf
scripts/kconfig/conf  --olddefconfig Kconfig
#
# configuration written to .config
#
Starting Build
scripts/kconfig/conf  --syncconfig Kconfig
  SYSTBL  arch/x86/include/generated/asm/syscalls_32.h
  SYSHDR  arch/x86/include/generated/asm/unistd_32_ia32.h
  SYSHDR  arch/x86/include/generated/asm/unistd_64_x32.h
  SYSTBL  arch/x86/include/generated/asm/syscalls_64.h
--
  LD [M]  sound/usb/usx2y/snd-usb-usx2y.ko
  LD [M]  sound/virtio/virtio_snd.ko
  LD [M]  sound/x86/snd-hdmi-lpe-audio.ko
  LD [M]  sound/xen/snd_xen_front.ko
  LD [M]  virt/lib/irqbypass.ko
[TIMER]{BUILD}: 1923s
Making Modules
  INSTALL arch/x86/crypto/blowfish-x86_64.ko
  INSTALL arch/x86/crypto/camellia-aesni-avx-x86_64.ko
  INSTALL arch/x86/crypto/camellia-aesni-avx2.ko
  INSTALL arch/x86/crypto/camellia-x86_64.ko
--
  INSTALL sound/virtio/virtio_snd.ko
  INSTALL sound/x86/snd-hdmi-lpe-audio.ko
  INSTALL sound/xen/snd_xen_front.ko
  INSTALL virt/lib/irqbypass.ko
  DEPMOD  4.18.0-jmaple_fips-8-compliant_4.18.0-553.16.1-f51d42ab35ac+
[TIMER]{MODULES}: 15s
Making Install
sh ./arch/x86/boot/install.sh 4.18.0-jmaple_fips-8-compliant_4.18.0-553.16.1-f51d42ab35ac+ arch/x86/boot/bzImage \
        System.map "/boot"
[TIMER]{INSTALL}: 22s
Checking kABI
kABI check passed
Setting Default Kernel to /boot/vmlinuz-4.18.0-jmaple_fips-8-compliant_4.18.0-553.16.1-f51d42ab35ac+ and Index to 0
Hopefully Grub2.0 took everything ... rebooting after time metrices
[TIMER]{MRPROPER}: 5s
[TIMER]{BUILD}: 1923s
[TIMER]{MODULES}: 15s
[TIMER]{INSTALL}: 22s
[TIMER]{TOTAL} 1969s
Rebooting in 10 seconds

KSelfTest

[jmaple@devbox code]$ ./get_kselftest_diff.sh
kselftest.4.18.0-jmaple_batch_12_fips-8-compliant_4.18.0-553.16.1-d70e+.log
204
kselftest.4.18.0-jmaple_batch_12_fips-8-compliant_4.18.0-553.16.1-9a06+.log
204
kselftest.4.18.0-jmaple_udp_gso_fraglist-106adb1d0a8f+.log
204
kselftest.4.18.0-jmaple_fips-8-compliant_4.18.0-553.16.1-f51d42ab35ac+.log
204
Before: kselftest.4.18.0-jmaple_udp_gso_fraglist-106adb1d0a8f+.log
After: kselftest.4.18.0-jmaple_fips-8-compliant_4.18.0-553.16.1-f51d42ab35ac+.log
Diff:
No differences found.

KselfTest Diff Experimental

#!/bin/bash

FILES=$(ls -rt kselftest.* | tail -n4)

while read -r line; do
        echo $line; grep '^ok ' $line | wc -l ;
done <<< "$FILES"

BEFORE=""
AFTER+=""

while read -r line; do
    BEFORE=${AFTER}
    AFTER=${line}
done <<< "$FILES"

echo "Before: $BEFORE"
echo "After: $AFTER"
echo "Diff:"
DIFF=$(grep ok <(diff -adU0 <(grep ^ok "${BEFORE}" | sort -h) <(grep ^ok "${AFTER}" | sort -h)))
if [ -z "$DIFF" ]; then
    echo "No differences found."
else
    echo "$DIFF"
fi

jira VULN-53462
jira VULN-53461
cve-pre CVE-2024-58002
commit-author Ricardo Ribalda <ribalda@chromium.org>
commit 64627da

Avoid using the iterators after the list_for_each() constructs.
This patch should be a NOP, but makes cocci, happier:

drivers/media/usb/uvc/uvc_ctrl.c:1861:44-50: ERROR: invalid reference to the index variable of the iterator on line 1850
drivers/media/usb/uvc/uvc_ctrl.c:2195:17-23: ERROR: invalid reference to the index variable of the iterator on line 2179

	Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
	Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
	Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
	Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
(cherry picked from commit 64627da)
	Signed-off-by: Jonathan Maple <jmaple@ciq.com>
jira VULN-53462
jira VULN-53461
cve-pre CVE-2024-58002
commit-author Ricardo Ribalda <ribalda@chromium.org>
commit d9fecd0

Now we keep a reference to the active fh for any call to uvc_ctrl_set,
regardless if it is an actual set or if it is a just a try or if the
device refused the operation.

We should only keep the file handle if the device actually accepted
applying the operation.

	Cc: stable@vger.kernel.org
Fixes: e5225c8 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
	Suggested-by: Hans de Goede <hdegoede@redhat.com>
	Reviewed-by: Hans de Goede <hdegoede@redhat.com>
	Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
	Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Link: https://lore.kernel.org/r/20241203-uvc-fix-async-v6-1-26c867231118@chromium.org
	Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
	Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
(cherry picked from commit d9fecd0)
	Signed-off-by: Jonathan Maple <jmaple@ciq.com>
jira VULN-53462
jira VULN-53461
cve CVE-2024-58002
commit-author Ricardo Ribalda <ribalda@chromium.org>
commit 221cd51
upstream-diff used kernel-lt 5.15 commit 117f7a2
	This is due to missing both:
	 - e8c0708 - Kbuild: move to -std=gnu11
	 - 54da6a0 - locking: Introduce __cleanup() based infrastructure

When an async control is written, we copy a pointer to the file handle
that started the operation. That pointer will be used when the device is
done. Which could be anytime in the future.

If the user closes that file descriptor, its structure will be freed,
and there will be one dangling pointer per pending async control, that
the driver will try to use.

Clean all the dangling pointers during release().

To avoid adding a performance penalty in the most common case (no async
operation), a counter has been introduced with some logic to make sure
that it is properly handled.

	Cc: stable@vger.kernel.org
Fixes: e5225c8 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
	Reviewed-by: Hans de Goede <hdegoede@redhat.com>
	Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
	Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://lore.kernel.org/r/20241203-uvc-fix-async-v6-3-26c867231118@chromium.org
	Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
	Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
	Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 117f7a2)
	Signed-off-by: Jonathan Maple <jmaple@ciq.com>
Copy link
Collaborator

@bmastbergen bmastbergen left a comment

Choose a reason for hiding this comment

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

🥌

@roxanan1996
Copy link

It looks correct.

But I have some questions related to the third commit.

  1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?
  2. I used check_kernel_commits for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
    but in this case, the commits for this pull request have 2 origins (mainline and stable).
    In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to cherry picked from ... check point 3..
    Plus, it improves visibility but I may be biased because I got used to this.
  3. I also used interdiff in the check_kernel_commits without any extra changes. For patch 1 and 2 it works perfectly.
    For patch 3, it does not.
    It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
    But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

@roxanan1996
Copy link

It looks correct.

But I have some questions related to the third commit.

1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?

2. I used [check_kernel_commits](https://github.com/ctrliq/kernel-src-tree-tools/blob/mainline/check_kernel_commits.py) for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
   but in this case, the commits for this pull request have 2 origins (mainline and stable).
   In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to `cherry picked from ...` [check point 3.](https://canonical-kernel-docs.readthedocs-hosted.com/latest/reference/stable-patch-format/#comment-body).
   Plus, it improves visibility but I may be biased because I got used to this.

This is already included in the commit message, but I am not sure if this follows a specific format. It gave me the impression you wrote it yourself.

3. I also used interdiff in the `check_kernel_commits`  without any extra changes. For patch 1 and 2 it works perfectly.
   For patch 3, it does not.
   It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
   But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

@bmastbergen
Copy link
Collaborator

  1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?

You are right, we don't have a common reference to all LTs in this repo, only 6.12.y right now. When we backport from other LTs we are just using local references to those trees.

  1. I used check_kernel_commits for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
    but in this case, the commits for this pull request have 2 origins (mainline and stable).
    In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to cherry picked from ... check point 3..
    Plus, it improves visibility but I may be biased because I got used to this.

We like having the reference to the mainline commit in there, but I do think it would be a good idea to codify when we cherry-pick from someplace else. Right now its just a free form thing in the upstream-diff section, but if we came up with a defined way to reference this then tooling like check_kernel_commits could use it.

  1. I also used interdiff in the check_kernel_commits without any extra changes. For patch 1 and 2 it works perfectly.
    For patch 3, it does not.
    It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
    But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

Same as above, if we had a codified way to call out backports from places other than mainline, then interdiff based tooling could use it too.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

It does! Feel free to make PRs to kernel-src-tree-tools to improve/extend check_kernel_commits. Thanks!

@PlaidCat
Copy link
Collaborator Author

PlaidCat commented Oct 1, 2025

It looks correct.

But I have some questions related to the third commit.

1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?

2. I used [check_kernel_commits](https://github.com/ctrliq/kernel-src-tree-tools/blob/mainline/check_kernel_commits.py) for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
   but in this case, the commits for this pull request have 2 origins (mainline and stable).
   In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to `cherry picked from ...` [check point 3.](https://canonical-kernel-docs.readthedocs-hosted.com/latest/reference/stable-patch-format/#comment-body).
   Plus, it improves visibility but I may be biased because I got used to this.

3. I also used interdiff in the `check_kernel_commits`  without any extra changes. For patch 1 and 2 it works perfectly.
   For patch 3, it does not.
   It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
   But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

  1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?

You are right, we don't have a common reference to all LTs in this repo, only 6.12.y right now. When we backport from other LTs we are just using local references to those trees.

  1. I used check_kernel_commits for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
    but in this case, the commits for this pull request have 2 origins (mainline and stable).
    In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to cherry picked from ... check point 3..
    Plus, it improves visibility but I may be biased because I got used to this.

We like having the reference to the mainline commit in there, but I do think it would be a good idea to codify when we cherry-pick from someplace else. Right now its just a free form thing in the upstream-diff section, but if we came up with a defined way to reference this then tooling like check_kernel_commits could use it.

  1. I also used interdiff in the check_kernel_commits without any extra changes. For patch 1 and 2 it works perfectly.
    For patch 3, it does not.
    It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
    But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

Same as above, if we had a codified way to call out backports from places other than mainline, then interdiff based tooling could use it too.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

It does! Feel free to make PRs to kernel-src-tree-tools to improve/extend check_kernel_commits. Thanks!

Both what @roxanan1996 and @bmastbergen said is correct. The reason, to Brett's point, on why we don't mirror all the LT branches into this repo is that this repo is already nearly 6GiB checkout. Additionally Red Hat has backported 120,000+ commits from Mainline into this "4.18.0" kernel. Check our rocky8_10 and then do a git log --oneline v4.18..HEAD | wc -l ... thats with some non trivail changes from Red Hat as well. So even if the LT version is "close" its not always applicable due to how aggressive RedHat is with backporting.

We have a default formatting here: https://github.com/ctrliq/kernel-src-tree/wiki#commit-message-header
the upstream-diff is a hand written note for reviewers AND for people doing investigation / git bisect | git blame.
In addition we have used several pieces of tooling that specifically looks for ^commit <upstream sha> inside the message body prior to the first empty line, kernel LT and CentOS-streams each have their own way of doing this for referencing where in the upstream a commit came from we just standardized on this when we started this so that we had a solid point into mainline and might be able to find bugfixes in the mainline that might be ignored if we just used a kernel-LT version.
For example on where this could get messed up, in Rocky9.6 somewhere between 9.5 and 9.6 RedHat / CentOS synced the 5.14.0 Intel ICE driver to the version in 6.12, and they've synced it several times since then so 9.7 in November will be somewhere around 6.17 Im thinking. See this link as a point of reference: https://forums.rockylinux.org/t/network-tx-drops-now-non-zero-after-update-to-rl9-6/19592/15?u=plaidcat

WRT to the cherry-pick line, you're 100% correct I forgot about this aspect of the commit process.

(cherry picked from commit 117f7a2975baa4b7d702d3f4830d5a4ebd0c6d50)
	Signed-off-by: Jonathan Maple <jmaple@ciq.com>`

As with what Brett said maybe we can expand ciq-cherry-pick with --klt <version> and it will know to locally add and fetch stable, automatically fix up the commit <lt> -> commit <mainline sha> and add upstream-diff kernel-lt <version>. Then expand interdiff scripts to look for this as you suggested.

Open to suggestions and its a good set of comments thanks for diggin into the commit and content.

@PlaidCat PlaidCat merged commit f51d42a into fips-8-compliant/4.18.0-553.16.1 Oct 1, 2025
4 checks passed
@PlaidCat PlaidCat deleted the {jmaple}_fips-8-compliant/4.18.0-553.16.1 branch October 1, 2025 18:19
@roxanan1996
Copy link

It looks correct.
But I have some questions related to the third commit.

1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?

2. I used [check_kernel_commits](https://github.com/ctrliq/kernel-src-tree-tools/blob/mainline/check_kernel_commits.py) for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
   but in this case, the commits for this pull request have 2 origins (mainline and stable).
   In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to `cherry picked from ...` [check point 3.](https://canonical-kernel-docs.readthedocs-hosted.com/latest/reference/stable-patch-format/#comment-body).
   Plus, it improves visibility but I may be biased because I got used to this.

3. I also used interdiff in the `check_kernel_commits`  without any extra changes. For patch 1 and 2 it works perfectly.
   For patch 3, it does not.
   It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
   But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

  1. The actual commit is from 5.15 stable. But the commit is not reachable from this repo, right?

You are right, we don't have a common reference to all LTs in this repo, only 6.12.y right now. When we backport from other LTs we are just using local references to those trees.

  1. I used check_kernel_commits for reviewing, which uses a common upstream reference (default is origin/kernel-mainline),
    but in this case, the commits for this pull request have 2 origins (mainline and stable).
    In my opinion, the upstream reference should be taken from the actual commit message (Canonical does that next to cherry picked from ... check point 3..
    Plus, it improves visibility but I may be biased because I got used to this.

We like having the reference to the mainline commit in there, but I do think it would be a good idea to codify when we cherry-pick from someplace else. Right now its just a free form thing in the upstream-diff section, but if we came up with a defined way to reference this then tooling like check_kernel_commits could use it.

  1. I also used interdiff in the check_kernel_commits without any extra changes. For patch 1 and 2 it works perfectly.
    For patch 3, it does not.
    It compares against the commit from mainline, not stable 5.15. I think this is beneficial, so you can see the actual diff. And why you had to cherry-pick the commit from stable.
    But then it misses the comparison against the actual commit that's cherry-picked from stable. So this has to be added.

Same as above, if we had a codified way to call out backports from places other than mainline, then interdiff based tooling could use it too.

I hope this makes sense. Let me know what you think. I believe this could be solved in check_kernel_commits script easily and I would be happy to do that.

It does! Feel free to make PRs to kernel-src-tree-tools to improve/extend check_kernel_commits. Thanks!

Both what @roxanan1996 and @bmastbergen said is correct. The reason, to Brett's point, on why we don't mirror all the LT branches into this repo is that this repo is already nearly 6GiB checkout. Additionally Red Hat has backported 120,000+ commits from Mainline into this "4.18.0" kernel. Check our rocky8_10 and then do a git log --oneline v4.18..HEAD | wc -l ... thats with some non trivail changes from Red Hat as well. So even if the LT version is "close" its not always applicable due to how aggressive RedHat is with backporting.

We have a default formatting here: https://github.com/ctrliq/kernel-src-tree/wiki#commit-message-header the upstream-diff is a hand written note for reviewers AND for people doing investigation / git bisect | git blame. In addition we have used several pieces of tooling that specifically looks for ^commit <upstream sha> inside the message body prior to the first empty line, kernel LT and CentOS-streams each have their own way of doing this for referencing where in the upstream a commit came from we just standardized on this when we started this so that we had a solid point into mainline and might be able to find bugfixes in the mainline that might be ignored if we just used a kernel-LT version. For example on where this could get messed up, in Rocky9.6 somewhere between 9.5 and 9.6 RedHat / CentOS synced the 5.14.0 Intel ICE driver to the version in 6.12, and they've synced it several times since then so 9.7 in November will be somewhere around 6.17 Im thinking. See this link as a point of reference: https://forums.rockylinux.org/t/network-tx-drops-now-non-zero-after-update-to-rl9-6/19592/15?u=plaidcat

WRT to the cherry-pick line, you're 100% correct I forgot about this aspect of the commit process.

(cherry picked from commit 117f7a2975baa4b7d702d3f4830d5a4ebd0c6d50)
	Signed-off-by: Jonathan Maple <jmaple@ciq.com>`

As with what Brett said maybe we can expand ciq-cherry-pick with --klt <version> and it will know to locally add and fetch stable, automatically fix up the commit <lt> -> commit <mainline sha> and add upstream-diff kernel-lt <version>. Then expand interdiff scripts to look for this as you suggested.

Open to suggestions and its a good set of comments thanks for diggin into the commit and content.

Thanks for your feedback. I will create a jira ticket (still need to figure out where) with a summary of the discussion and plan of attack.

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

Successfully merging this pull request may close these issues.

4 participants