linux-kernel - Re: [for-next][PATCH 23/24] string.h: Add strncmp

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] [day] [month] [year] [list]

Message-ID: <20181221180348.766f7712@gandalf.local.home>
Date:   Fri, 21 Dec 2018 18:03:48 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Joe Perches <joe@...ches.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper
 macro

On Fri, 21 Dec 2018 14:57:13 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt <rostedt@...dmis.org> wrote:
> >  
> > > Your patch actually had them, but in the body of your email you did
> > >  
> > > > #define have_prefix(str, prefix) ({ \
> > > >       const char *__pfx = (const char *)prefix; \  
> > >
> > > which is just completely wrong.
> > >
> > > Considering your _old_ patch had the exact same bug, I really think
> > > you need to start internalizing the whole "macro arguments *have* to
> > > be properly protected" thing.  
> >
> > Well, there's less with assignments that can go wrong than with other
> > code. That is, there's little that can happen with "int x = arg;" where
> > arg is the macro paramater to cause something really nasty.  
> 
> What's wrong, Steven?

We are miscommunicating here...

I was talking about the missing parenthesis on the original patch,
which you stated was missing as well. And the original patch didn't
have the typecast.

> 
> The assignment is entirely irrelevant.
> 
> The problem is the cast.
> 
> A type cast has a very high priority, and so if you do
> 
>     (const char *)prefix
> 
> it breaks completely if you might have something like"a+6" as the argument.
> 
> Think about what if "a" is of type "unsigned long", for example?

Yes, when writing the real code, I noticed that the typecast could
cause issues without the parenthesis, and added them.

The email you are replying to was saying there's not much that can go
wrong with:

#define MACRO(x) { \
	int __p = x; \


I definitely can see something wrong with:

#define MACRO(x) { \
	int __p = (int)x; \

because exactly what you stated.

There's nothing wrong with adding (x) for the first one, but it's
unlikely anything will cause it harm. The second one the (x) *is* most
definitely required.

This is a long winded "I agree with you" ;-)

-- Steve