linux-kernel - Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

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: <1420046240.2085.3.camel@HansenPartnership.com>
Date:	Wed, 31 Dec 2014 09:17:20 -0800
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	Helge Deller <deller@....de>, linux-kernel@...r.kernel.org,
	Arnd Bergmann <arnd@...db.de>, linux-arch@...r.kernel.org,
	"James E.J. Bottomley" <jejb@...isc-linux.org>,
	linux-parisc@...r.kernel.org
Subject: Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

On Sat, 2014-12-27 at 18:14 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 25, 2014 at 11:37:45PM +0100, Helge Deller wrote:
> > Hi Michael,
> > 
> > On 12/25/2014 10:29 AM, Michael S. Tsirkin wrote:
> > >virtio wants to read bitwise types from userspace using get_user.  At the
> > 
> > I don't know the virtio code much yet, but does it makes sense to read bitwise types?
> > Will virtio then get possible troubles because of endianess correct as well?
> 
> There's no conversion: we are reading from __virtio16 __user *
> pointer into __virtio16 v value.
> 
> > Do you have a code example, or the sparse error message ?
> > 
> > Helge
> 
> Sure. the code is upstream now.
> The warning is below.
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/vhost/vringh.c:554:18: sparse: cast to restricted __virtio16
> 
> vim +554 drivers/vhost/vringh.c
> 
>    538                                                           __virtio16 *p, u16 val))
>    539  {
>    540          if (!vrh->event_indices) {
>    541                  /* Old-school; update flags. */
>    542                  if (putu16(vrh, &vrh->vring.used->flags,
>    543                             VRING_USED_F_NO_NOTIFY)) {
>    544                          vringh_bad("Setting used flags %p",
>    545                                     &vrh->vring.used->flags);
>    546                  }
>    547          }
>    548  }
>    549
>    550  /* Userspace access helpers: in this case, addresses are really userspace. */
>    551  static inline int getu16_user(const struct vringh *vrh, u16 *val, const __virtio16 *p)
>    552  {
>    553          __virtio16 v = 0;
>  > 554          int rc = get_user(v, (__force __virtio16 __user *)p);
>    555          *val = vringh16_to_cpu(vrh, v);
>    556          return rc;
>    557  }
>    558
>    559  static inline int putu16_user(const struct vringh *vrh, __virtio16 *p, u16 val)
>    560  {
>    561          __virtio16 v = cpu_to_vringh16(vrh, val);
>    562          return put_user(v, (__force __virtio16 __user *)p);

OK, parisc developers still being dense, but this does look like an
abuse of the bitwise type.  bitwise is supposed to be consumed by endian
specific accessors.  get/put_user have no endian tags because they
really can't do this ... the potential for width mismatch between the
user and kernel address spaces could cause havoc if people get this
wrong, so the warning looks correct to me.

If we take your proposed patch we lose the type checking on all
accessors because of the __force.  Why not, instead, alter your code to
tell the kernel you know what you're doing:

        __u16 v = 0;
        int rc = get_user(v, (__force __u16 __user *)p);
        *val = vringh16_to_cpu(vrh, (__force __virtio16)v);
        return rc;

That way the accessors still warn if anyone else tries this but your
warning is gone and the code basically says you knew the u16 was really
an endianness specific virtio quantity?

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/