linux-kernel - Re: [PATCH 1/1] Add virtio-input driver.

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, 23 Mar 2015 08:53:58 +0100
From:	Gerd Hoffmann <kraxel@...hat.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	virtio-dev@...ts.oasis-open.org,
	virtualization@...ts.linux-foundation.org,
	Rusty Russell <rusty@...tcorp.com.au>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:ABI/API" <linux-api@...r.kernel.org>
Subject: Re: [PATCH 1/1] Add virtio-input driver.

  Hi,

> > > > +		if (cfg & (1 << (bit % 8)))
> > > > +			set_bit(bit, bits);
> > > 
> > > what if not set? does something clear the mask?
> > 
> > kzalloc?
> 
> So you are really just reading in array of bytes?
> All this set bit trickery is just to convert things from LE?

Trickery?  Just checking each bit from virtio config space, then set it
in the input layer bitmap.  It's a simple stupid loop.

Surely not the most efficient way, but hey, it's not in the hot path and
I'm sure I'm setting the bits correctly because this uses the standard
linux kernel bitops.

> At least, this needs a comment explaining what the function does,
> and maybe wrap it in a helper like virtio_input_bitmap_copy or
> virtio_bitmap_or.

Can do that, sure.

> > > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > > 
> > > you read le field into u32 value.
> > > Please run sparse on this code. you will get a ton
> > > of warnings. Same error appears elsewhere.
> > 
> > Indeed.  IIRC that wasn't the case a while back.  Guess those bitwise
> > annotations have been added with the virtio 1.0 patches?
> > 
> > In any case I'll fix it up.
> 
> I see you still didn't in v2?

v2 builds fine without sparse warnings.  virtio_cread handles swapping
if needed and returns native endian, so I have to store this in normal
u32 variables and pass it on to the input layer as-is.

> You are doing leXXX everywhere, that's VERSION_1 dependency.
> virtio_cread will do byteswaps differently without VERSION_1.
> Just don't go there.

Changed that for v2, for the config space structs.  They have normal u32
in there now.  virtio_cread() wants it this way.

cheers,
  Gerd


--
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/