linux-kernel - Re: Build fail in drivers/gpu/drm/i915/display/intel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZVPFUdc6Q/qCI8M7@ideak-desk.fi.intel.com>
Date:   Tue, 14 Nov 2023 21:07:45 +0200
From:   Imre Deak <imre.deak@...el.com>
To:     David Laight <David.Laight@...lab.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

On Fri, Nov 10, 2023 at 09:00:21AM +0000, David Laight wrote:
> From: Linus Torvalds
> > Sent: 10 November 2023 00:52
> > 
> > On Thu, 9 Nov 2023 at 15:34, Imre Deak <imre.deak@...el.com> wrote:
> > >
> > > The compiler warn should be fixed/suppressed by:
> > > https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@intel.com
> > 
> > Ugh, so now it's a dynamic allocation, wasting memory, and a pointer
> > to it, using as much memory as the array did in the first place.
> > 
> > All because of a pointless warning that was a false positive - and was
> > always harmless anyway, since snprintf() is safe (ie it was only a
> > "might be truncated").
> 
> That entire warning for snprintf() is a false positive.
> The ones that are likely to overflow unexpectedly are the ones
> with a "%s" format for a 'char *' pointer where there is no
> implied length.
> 
> The same check for printf() using the implied buffer length
> probably does make sense.
> 
> I don't even think there is a way of avoiding it on a case by case
> basis - apart from passing both the buffer address and length
> to an inline asm that the compiler has to assume might change
> the values, but that tends to generate an extra 'mov' instruction
> for no good reason at all.
> 
> > 
> > Please don't do this. Either do that ((tc_port & 7) + 1) suggestion of
> > David's, or just do '%c' and make the expression be
> > 
> >   '1' + tc_port
> > 
> > which should be fine since I915_MAX_PORTS is 8 or whatever.

Ok, the patch above was merged already to drm-tip, but I agree not to
use kasprintf for this. The above looks ok and there is already
tc_port_name() for this, would just need to export that from
intel_display.h.

I can follow up with a patch for this, or if you or David wanted to do
that please send a patch to the intel-gfx@...ts.freedesktop.org list.

Thanks,
Imre

> If I'd though for 2 seconds that is what I'd have done.
> But I wanted to get something through the compiler.
> 
> > I do wonder why those ports are printed out as '1-8', when the 'enum
> > port' is PORT_A..I.
> 
> They look like TC_PORT_[1..6] to me - the enum values are 0..5
> which is why there is a 'random' '+ 1'.
> 
> 	David
> 
> > 
> > So it would actually have made more sense to print them out as %c with
> > the expression being
> > 
> >    'A'+tc_port
> > 
> > but I guess by now people might depend on the 1..8 naming?
> > 
> >              Linus
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)