Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Herbert Xu]

On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
>
> +	size = min_t(unsigned int, size, avail - vi->data_idx);
> +	idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> +	memcpy(buf, vi->data + idx, size);

I don't see how nospec can help here.  Please enlighten me.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> >
> > +	size = min_t(unsigned int, size, avail - vi->data_idx);
> > +	idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> > +	memcpy(buf, vi->data + idx, size);
> 
> I don't see how nospec can help here.  Please enlighten me.


All the "malicious device" things are confusing. Spectre things -
doubly so.

So if an access is speculated then CPU might speculate feeding a kernel
secret into RNG. And then the speculated RNG value maybe can be also
speculatively be used by some kernel code as an index
to trigger a cache access, finally leaking the secret?

Maybe?




> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Herbert Xu]

On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > >
> > > +	size = min_t(unsigned int, size, avail - vi->data_idx);
> > > +	idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> > > +	memcpy(buf, vi->data + idx, size);
> 
> All the "malicious device" things are confusing. Spectre things -
> doubly so.
> 
> So if an access is speculated then CPU might speculate feeding a kernel
> secret into RNG. And then the speculated RNG value maybe can be also
> speculatively be used by some kernel code as an index
> to trigger a cache access, finally leaking the secret?
> 
> Maybe?

The way Spectre works is if you have an actual instruction using
idx directly.  I don't see how that translates to memcpy.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 03:46:58PM +0800, Herbert Xu wrote:
> On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > > >
> > > > +	size = min_t(unsigned int, size, avail - vi->data_idx);
> > > > +	idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> > > > +	memcpy(buf, vi->data + idx, size);
> > 
> > All the "malicious device" things are confusing. Spectre things -
> > doubly so.
> > 
> > So if an access is speculated then CPU might speculate feeding a kernel
> > secret into RNG. And then the speculated RNG value maybe can be also
> > speculatively be used by some kernel code as an index
> > to trigger a cache access, finally leaking the secret?
> > 
> > Maybe?
> 
> The way Spectre works is if you have an actual instruction using
> idx directly.  I don't see how that translates to memcpy.

I am not sure it has to be direct:

if (malicious_idx > SIZE)
	return;
src += malicious_idx;
memcpy(&value, src, ...)
....
hash = complex_hash_of(value)
....
return p[hash * 512];

is IIUC still a valid spectre v1 gadget leaking a value beyong SIZE, or
did I miss something?


And rng is a kind of a complex hash, but I also think in that "...."
in the kernel is probably large enough to close any transient execution
window.


So sure, we can drop this.




> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Herbert Xu]

On Thu, Jun 11, 2026 at 03:58:17AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 11, 2026 at 03:46:58PM +0800, Herbert Xu wrote:
> > On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > > > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > > > >
> > > > > +	size = min_t(unsigned int, size, avail - vi->data_idx);
> > > > > +	idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> > > > > +	memcpy(buf, vi->data + idx, size);
> > > 
> > > All the "malicious device" things are confusing. Spectre things -
> > > doubly so.
> > > 
> > > So if an access is speculated then CPU might speculate feeding a kernel
> > > secret into RNG. And then the speculated RNG value maybe can be also
> > > speculatively be used by some kernel code as an index
> > > to trigger a cache access, finally leaking the secret?
> > > 
> > > Maybe?
> > 
> > The way Spectre works is if you have an actual instruction using
> > idx directly.  I don't see how that translates to memcpy.
> 
> I am not sure it has to be direct:
> 
> if (malicious_idx > SIZE)
> 	return;
> src += malicious_idx;

Wait but vi->data_idx isn't even under the hypervisor's control.

It's an index maintained by our own driver.  So how can it be
malicious?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 04:18:46PM +0800, Herbert Xu wrote:
> On Thu, Jun 11, 2026 at 03:58:17AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 11, 2026 at 03:46:58PM +0800, Herbert Xu wrote:
> > > On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > > > > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > > > > >
> > > > > > +	size = min_t(unsigned int, size, avail - vi->data_idx);
> > > > > > +	idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> > > > > > +	memcpy(buf, vi->data + idx, size);
> > > > 
> > > > All the "malicious device" things are confusing. Spectre things -
> > > > doubly so.
> > > > 
> > > > So if an access is speculated then CPU might speculate feeding a kernel
> > > > secret into RNG. And then the speculated RNG value maybe can be also
> > > > speculatively be used by some kernel code as an index
> > > > to trigger a cache access, finally leaking the secret?
> > > > 
> > > > Maybe?
> > > 
> > > The way Spectre works is if you have an actual instruction using
> > > idx directly.  I don't see how that translates to memcpy.
> > 
> > I am not sure it has to be direct:
> > 
> > if (malicious_idx > SIZE)
> > 	return;
> > src += malicious_idx;
> 
> Wait but vi->data_idx isn't even under the hypervisor's control.
> 
> It's an index maintained by our own driver.  So how can it be
> malicious?
> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


data_avail is under hypervisor control

        avail = min_t(unsigned int, vi->data_avail, sizeof(vi->data));
        if (vi->data_idx >= avail) {
        	vi->data_idx = 0;

and maybe this can speculate past the if?

I agree, this is all speculation )


-- 
MST

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Herbert Xu]

On Thu, Jun 11, 2026 at 05:10:32AM -0400, Michael S. Tsirkin wrote:
>
> data_avail is under hypervisor control
> 
>         avail = min_t(unsigned int, vi->data_avail, sizeof(vi->data));
>         if (vi->data_idx >= avail) {
>         	vi->data_idx = 0;
> 
> and maybe this can speculate past the if?
> 
> I agree, this is all speculation )

Either it is vulnerable to Spectre, or it isn't.  Adding nospec
markers when you're not sure is cargo cult programming.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] hwrng: virtio: clamp device-reported used.len at copy_data()

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 05:19:26PM +0800, Herbert Xu wrote:
> On Thu, Jun 11, 2026 at 05:10:32AM -0400, Michael S. Tsirkin wrote:
> >
> > data_avail is under hypervisor control
> > 
> >         avail = min_t(unsigned int, vi->data_avail, sizeof(vi->data));
> >         if (vi->data_idx >= avail) {
> >         	vi->data_idx = 0;
> > 
> > and maybe this can speculate past the if?
> > 
> > I agree, this is all speculation )
> 
> Either it is vulnerable to Spectre, or it isn't.  Adding nospec
> markers when you're not sure is cargo cult programming.

AKA defence is depth programming)
Alright we can drop this. No biggie.

-- 
MST