[PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[Josh Poimboeuf]

The barrier_nospec() after the array bounds check is overkill and
painfully slow for arches which implement it.

Furthermore, most arches don't implement it, so they remain exposed to
Spectre v1 (which can affect pretty much any CPU with branch
prediction).

Instead, clamp the user pointer to a valid range so it's guaranteed to
be a valid array index even when the bounds check mispredicts.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 drivers/cdrom/cdrom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 9b0f37d4b9d4..6a99a459b80b 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2313,7 +2313,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 		return -EINVAL;
 
 	/* Prevent arg from speculatively bypassing the length check */
-	barrier_nospec();
+	arg = array_index_nospec(arg, cdi->capacity);
 
 	info = kmalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
-- 
2.47.0

Re: [PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[Jens Axboe]

On 10/17/24 4:09 PM, Josh Poimboeuf wrote:
> The barrier_nospec() after the array bounds check is overkill and
> painfully slow for arches which implement it.
> 
> Furthermore, most arches don't implement it, so they remain exposed to
> Spectre v1 (which can affect pretty much any CPU with branch
> prediction).
> 
> Instead, clamp the user pointer to a valid range so it's guaranteed to
> be a valid array index even when the bounds check mispredicts.

It's a cdrom, and media change detection to be more specific. I really
don't think anyone would care about performance here, it's not even
a hot path for a cdrom driver. That said, I don't disagree with
the change, just don't think it'll make one iota of difference
in the real world.

-- 
Jens Axboe

Re: [PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[Josh Poimboeuf]

On Thu, Oct 17, 2024 at 06:33:24PM -0600, Jens Axboe wrote:
> On 10/17/24 4:09 PM, Josh Poimboeuf wrote:
> > The barrier_nospec() after the array bounds check is overkill and
> > painfully slow for arches which implement it.
> > 
> > Furthermore, most arches don't implement it, so they remain exposed to
> > Spectre v1 (which can affect pretty much any CPU with branch
> > prediction).
> > 
> > Instead, clamp the user pointer to a valid range so it's guaranteed to
> > be a valid array index even when the bounds check mispredicts.
> 
> It's a cdrom, and media change detection to be more specific. I really
> don't think anyone would care about performance here, it's not even
> a hot path for a cdrom driver. That said, I don't disagree with
> the change, just don't think it'll make one iota of difference
> in the real world.

Fair, though it's also about hardening as barrier_nospec() is only
implemented by x86 and powerpc (see 2nd paragraph).  Most/all arches are
affected by Spectre v1.

-- 
Josh

Re: [PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[Jens Axboe]

On 10/17/24 6:52 PM, Josh Poimboeuf wrote:
> On Thu, Oct 17, 2024 at 06:33:24PM -0600, Jens Axboe wrote:
>> On 10/17/24 4:09 PM, Josh Poimboeuf wrote:
>>> The barrier_nospec() after the array bounds check is overkill and
>>> painfully slow for arches which implement it.
>>>
>>> Furthermore, most arches don't implement it, so they remain exposed to
>>> Spectre v1 (which can affect pretty much any CPU with branch
>>> prediction).
>>>
>>> Instead, clamp the user pointer to a valid range so it's guaranteed to
>>> be a valid array index even when the bounds check mispredicts.
>>
>> It's a cdrom, and media change detection to be more specific. I really
>> don't think anyone would care about performance here, it's not even
>> a hot path for a cdrom driver. That said, I don't disagree with
>> the change, just don't think it'll make one iota of difference
>> in the real world.
> 
> Fair, though it's also about hardening as barrier_nospec() is only
> implemented by x86 and powerpc (see 2nd paragraph).  Most/all arches are
> affected by Spectre v1.

Yep agree, if we don't have full arch coverage, then that's a better
reason for getting it included rather than performance reasons.

-- 
Jens Axboe

Re: [PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[Jens Axboe]

On Thu, 17 Oct 2024 15:09:02 -0700, Josh Poimboeuf wrote:
> The barrier_nospec() after the array bounds check is overkill and
> painfully slow for arches which implement it.
> 
> Furthermore, most arches don't implement it, so they remain exposed to
> Spectre v1 (which can affect pretty much any CPU with branch
> prediction).
> 
> [...]

Applied, thanks!

[1/1] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()
      commit: b0bf1afde7c34698cf61422fa8ee60e690dc25c3

Best regards,
-- 
Jens Axboe

Re: [PATCH] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()

[Phillip Potter]

On Thu, Oct 17, 2024 at 07:47:31PM -0600, Jens Axboe wrote:
> 
> On Thu, 17 Oct 2024 15:09:02 -0700, Josh Poimboeuf wrote:
> > The barrier_nospec() after the array bounds check is overkill and
> > painfully slow for arches which implement it.
> > 
> > Furthermore, most arches don't implement it, so they remain exposed to
> > Spectre v1 (which can affect pretty much any CPU with branch
> > prediction).
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] cdrom: Avoid barrier_nospec() in cdrom_ioctl_media_changed()
>       commit: b0bf1afde7c34698cf61422fa8ee60e690dc25c3
> 
> Best regards,
> -- 
> Jens Axboe
> 
> 
> 

Sorry for lack of reply, this conversation took place in the early hours
for my timezone and thus I was unconscious :-)

Change sounds good to me for the aforementioned reasons.

Regards,
Phil