linux-kernel - Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

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:   Thu, 24 Feb 2022 21:51:10 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Claudio Imbrenda <imbrenda@...ux.ibm.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        "open list:VFIO DRIVER" <kvm@...r.kernel.org>,
        linux-s390@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Janosch Frank <frankja@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>
Subject: Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <imbrenda@...ux.ibm.com> wrote:
>
> On Wed, 23 Feb 2022 18:44:20 +0200
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
>
> > While in this particular case it would not be a (critical) issue,
> > the pattern itself is bad and error prone in case somebody blindly
> > copies to their code.
> >
> > Don't cast parameter to unsigned long pointer in the bit operations.
> > Instead copy to a local variable on stack of a proper type and use.

...

> > +             struct { /* as a 256-bit bitmap */
> > +                     DECLARE_BITMAP(b, 256);
> > +             } bitmap;
> > +             struct { /* as a set of 64-bit words */
> >                       u64 word[4];
> >               } u64;

> > -     set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> > +     set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>
> wouldn't it be enough to pass gisa->u64.word here?
> then no cast would be necessary

No, it will have the same hidden bugs. As I stated in the commit
message, the pattern is quite bad even if in particular code it would
work.

Thanks, Michael, for pointing out other places. They all need to be fixed.

-- 
With Best Regards,
Andy Shevchenko