Kees,
I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and
in the process I got gcc-13 which is not WERROR-clean because we only
limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13
has all the same issues.
And I want to be able to do my arm64 builds with WERROR on still...
I guess it never made much sense to hope it was going to go away
without having a confirmation, so I just changed it to be gcc-11+.
A lot of the warnings seem just crazy, with gcc just not getting the
bounds right, and then being upset about us going backwards with
'container_of()' etc. Ok, so the kernel is special. We do odd things.
I get it, gcc ends up being confused.
But before I disabled it, I did take a look at a couple of warnings
that didn't look like the sea of crazy.
And one of them is from you.
In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix
nvif_outp_acquire_dp() argument size") cannot possibly be right, It
changes
nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16],
to
nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE],
and then does
memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd));
where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE
is 15.
So yeah, it's copying 16 bytes from an argument that claims to be 15
bytes in size.
I think that commit was wrong, and the problem is that the 'dpcd'
array is something 15 and sometimes 16. For example, we have
struct nouveau_encoder {
...
union {
struct {
...
u8 dpcd[DP_RECEIVER_CAP_SIZE];
} dp;
};
so there it's indeed 15 bytes, but then we have
union nvif_outp_acquire_args {
struct nvif_outp_acquire_v0 {
...
union {
...
struct {
...
__u8 dpcd[16];
} dp;
where it's 16.
I think it's all entirely harmless from a code generation standpoint,
because the 15-byte field will be padded out to 16 bytes in the
structure that contains it, but it's most definitely buggy.
So that warning does find real cases of wrong code. But when those
real cases are hidden by hundreds of lines of unfixable false
positives, we don't have much choice.
But could the Nouveau driver *please* pick a size for the dhcp[] array
and stick with it?
The other driver where the warnings didn't look entirely crazy was the
ath/carl9170 wireless driver, but I didn't look closer at that one.
Linus
On April 23, 2023 10:36:24 AM PDT, Linus Torvalds <torvalds at linux-foundation.org> wrote:>Kees, > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and >in the process I got gcc-13 which is not WERROR-clean because we only >limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13 >has all the same issues. > >And I want to be able to do my arm64 builds with WERROR on still... > >I guess it never made much sense to hope it was going to go away >without having a confirmation, so I just changed it to be gcc-11+.Yeah, that's fine. GCC 13 released without having a fix for at least one (hopefully last) known array-bounds vs jump threading bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071>And one of them is from you. > >In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix >nvif_outp_acquire_dp() argument size") cannot possibly be right, It >changes > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], > >to > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], > >and then does > > memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd)); > >where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15.Yeah, it was an incomplete fix. I sent the other half here, but it fell through the cracks: https://lore.kernel.org/lkml/20230204184307.never.825-kees at kernel.org/>>I think it's all entirely harmless from a code generation standpoint, >because the 15-byte field will be padded out to 16 bytes in the >structure that contains it, but it's most definitely buggy.Right; between this, that GCC 13 wasn't released yet, and I had no feedback from NV folks, I didn't chase down landing that fix.> >So that warning does find real cases of wrong code. But when those >real cases are hidden by hundreds of lines of unfixable false >positives, we don't have much choice.Yup, totally agreed. The false positives I've looked at all seem to be similar to the outstanding jump threading bug, so I'm hoping once that gets fixed we'll finally have a good signal with that warning enabled. :) -Kees -- Kees Cook
On Sun, Apr 23, 2023 at 10:24?PM Kees Cook <kees at kernel.org> wrote:> > On April 23, 2023 10:36:24 AM PDT, Linus Torvalds <torvalds at linux-foundation.org> wrote: > >Kees, > > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and > >in the process I got gcc-13 which is not WERROR-clean because we only > >limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13 > >has all the same issues. > > > >And I want to be able to do my arm64 builds with WERROR on still... > > > >I guess it never made much sense to hope it was going to go away > >without having a confirmation, so I just changed it to be gcc-11+. > > Yeah, that's fine. GCC 13 released without having a fix for at least one (hopefully last) known array-bounds vs jump threading bug: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071 > > >And one of them is from you. > > > >In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix > >nvif_outp_acquire_dp() argument size") cannot possibly be right, It > >changes > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], > > > >to > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], > > > >and then does > > > > memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd)); > > > >where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15. > > Yeah, it was an incomplete fix. I sent the other half here, but it fell through the cracks: > https://lore.kernel.org/lkml/20230204184307.never.825-kees at kernel.org/ >left a review on that patch. Any preferred way of getting it upstream? I could push it through drm-misc, but the delay until it reaches Linus' tree is a bit high.> > > > > > >I think it's all entirely harmless from a code generation standpoint, > >because the 15-byte field will be padded out to 16 bytes in the > >structure that contains it, but it's most definitely buggy. > > Right; between this, that GCC 13 wasn't released yet, and I had no feedback from NV folks, I didn't chase down landing that fix. > > > > >So that warning does find real cases of wrong code. But when those > >real cases are hidden by hundreds of lines of unfixable false > >positives, we don't have much choice. > > Yup, totally agreed. The false positives I've looked at all seem to be similar to the outstanding jump threading bug, so I'm hoping once that gets fixed we'll finally have a good signal with that warning enabled. :) > > -Kees > > > -- > Kees Cook >
Hey Linus, Kees. Responses below On Sun, 2023-04-23 at 13:23 -0700, Kees Cook wrote:> On April 23, 2023 10:36:24 AM PDT, Linus Torvalds <torvalds at linux-foundation.org> wrote: > > Kees, > > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and > > in the process I got gcc-13 which is not WERROR-clean because we only > > limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13 > > has all the same issues. > > > > And I want to be able to do my arm64 builds with WERROR on still... > > > > I guess it never made much sense to hope it was going to go away > > without having a confirmation, so I just changed it to be gcc-11+. > > Yeah, that's fine. GCC 13 released without having a fix for at least one (hopefully last) known array-bounds vs jump threading bug: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071 > > > And one of them is from you. > > > > In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix > > nvif_outp_acquire_dp() argument size") cannot possibly be right, It > > changes > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], > > > > to > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], > > > > and then does > > > > memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd)); > > > > where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15. > > Yeah, it was an incomplete fix. I sent the other half here, but it fell through the cracks: > https://lore.kernel.org/lkml/20230204184307.never.825-kees at kernel.org/Thanks for bringing this to our attention, yeah this definitely just looks like it got missed somewhere down the line. It looks like Karol responded already so I assume the patch is in the pipeline now, but let me know if there's anything else you need.> > > > > > > > I think it's all entirely harmless from a code generation standpoint, > > because the 15-byte field will be padded out to 16 bytes in the > > structure that contains it, but it's most definitely buggy. > > Right; between this, that GCC 13 wasn't released yet, and I had no feedback from NV folks, I didn't chase down landing that fix. > > > > > So that warning does find real cases of wrong code. But when those > > real cases are hidden by hundreds of lines of unfixable false > > positives, we don't have much choice. > > Yup, totally agreed. The false positives I've looked at all seem to be similar to the outstanding jump threading bug, so I'm hoping once that gets fixed we'll finally have a good signal with that warning enabled. :) > > -Kees > >-- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat