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]
Date:   Mon, 29 Apr 2019 13:29:50 +0200
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Edward Cree <ecree@...arflare.com>
Cc:     Nicholas Mc Guire <hofrat@...dl.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net_sched: force endianness annotation

On Mon, Apr 29, 2019 at 12:11:18PM +0100, Edward Cree wrote:
> On 29/04/2019 11:44, Nicholas Mc Guire wrote:
> > be16_to_cpu((__force __be16)val) should be a NOP on big-endian as well
> Yes.  But it's semiotically wrong to call be16_to_cpu() on a cpu-endian
>  value; if the existing behaviour is desired, it ought to be implemented
>  differently.
> > The problem with using swab16 is that it is impating the binary significantly
> > so I'm not sure if the change is really side-effect free
> It's not; it changes the behaviour.  That's why I brought up the question
>  of the intended behaviour ??? it's unclear whether the current (no-op on BE)
>  behaviour is correct or whether it's a bug in the original code.
> Better to leave the sparse error in place ??? drawing future developers'
>  attention to something being possibly wrong here ??? than to mask it with a
>  synthetic 'fix' which we don't even know if it's correct or not.
> 
> > but I just am unsure if
> > -                   val = be16_to_cpu(val);
> > +                   val = swab16(val);
> > is actually equivalent.
> If you're not sure about such things, maybe you shouldn't be touching
>  endianness-related code.  swab is *not* a no-op, either on BE or LE.

Well the only way to understand it is to try to understand it by reviewing
the implementatoins - which is whyt I'm currently doing - the principle 
issues are clear I think - following the details of the macro-chains is
not always that clear. From looking at the code history it does seem correct
which is why it seemed reasonable to remove the sparse warning and doing
so with a patch that does not change the binary seems the safest.

thx!
hofrat

Powered by blists - more mailing lists