* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
[not found] ` <20230612110040.849318-2-jordyzomer@google.com>
@ 2023-06-15 8:12 ` Phillip Potter
[not found] ` <20230615163125.td3aodpfwth5n4mc@desk>
2023-06-17 9:37 ` Phillip Potter
2 siblings, 0 replies; 7+ messages in thread
From: Phillip Potter @ 2023-06-15 8:12 UTC (permalink / raw
To: Jordy Zomer; +Cc: linux-kernel, linux-block
On Mon, Jun 12, 2023 at 11:00:40AM +0000, Jordy Zomer wrote:
> This patch fixes a spectre-v1 gadget in cdrom.
> The gadget could be triggered by,
> speculatviely bypassing the cdi->capacity check.
>
> Signed-off-by: Jordy Zomer <jordyzomer@google.com>
> ---
> drivers/cdrom/cdrom.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 416f723a2dbb..ecf2b458c108 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -264,6 +264,7 @@
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> +#include <linux/nospec.h>
> #include <linux/slab.h>
> #include <linux/cdrom.h>
> #include <linux/sysctl.h>
> @@ -2329,6 +2330,9 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> if (arg >= cdi->capacity)
> return -EINVAL;
>
> + /* Prevent arg from speculatively bypassing the length check */
> + barrier_nospec();
> +
> info = kmalloc(sizeof(*info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
> --
> 2.41.0.162.gfafddb0af9-goog
>
Hi Jordy,
Thanks for the patch, I will review/build it properly tonight after
work, although at first glance it looks good to me. I'll be in touch.
Regards,
Phil Potter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
[not found] ` <20230615163125.td3aodpfwth5n4mc@desk>
@ 2023-06-15 23:31 ` Phillip Potter
2023-06-16 3:14 ` Pawan Gupta
0 siblings, 1 reply; 7+ messages in thread
From: Phillip Potter @ 2023-06-15 23:31 UTC (permalink / raw
To: pawan.kumar.gupta; +Cc: linux-kernel, jordyzomer, linux-block
On Thu, Jun 15, 2023 at 09:31:25AM -0700, Pawan Gupta wrote:
> On Mon, Jun 12, 2023 at 11:00:40AM +0000, Jordy Zomer wrote:
> > This patch fixes a spectre-v1 gadget in cdrom.
> > The gadget could be triggered by,
> > speculatviely bypassing the cdi->capacity check.
> >
> > Signed-off-by: Jordy Zomer <jordyzomer@google.com>
> > ---
> > drivers/cdrom/cdrom.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> > index 416f723a2dbb..ecf2b458c108 100644
> > --- a/drivers/cdrom/cdrom.c
> > +++ b/drivers/cdrom/cdrom.c
> > @@ -264,6 +264,7 @@
> > #include <linux/errno.h>
> > #include <linux/kernel.h>
> > #include <linux/mm.h>
> > +#include <linux/nospec.h>
> > #include <linux/slab.h>
> > #include <linux/cdrom.h>
> > #include <linux/sysctl.h>
> > @@ -2329,6 +2330,9 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> > if (arg >= cdi->capacity)
> > return -EINVAL;
> >
> > + /* Prevent arg from speculatively bypassing the length check */
> > + barrier_nospec();
>
> On a quick look it at the call chain ...
>
> sr_block_ioctl(..., arg)
> cdrom_ioctl(..., arg)
> cdrom_ioctl_media_changed(..., arg)
>
> .... it appears maximum value cdi->capacity can be only 1:
>
> sr_probe()
> {
> ...
> cd->cdi.capacity = 1;
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/sr.c?h=v6.4-rc6#n665
>
> If we know that max possible value than, instead of big hammer
> barrier_nospec(), its possible to use lightweight array_index_nospec()
> as below:
> ...
Hi Pawan and Jordy,
I've now looked at this. It is possible for cdi->capacity to be > 1, as
it is set via get_capabilities() -> cdrom_number_of_slots(), if the
device is an individual or cartridge changer.
Therefore, I think using CDI_MAX_CAPACITY of 1 is not the correct
approach. Jordy's V2 patch is fine therefore, but perhaps using
array_index_nospec() with cdi->capacity is still better than a
do/while loop from a performance perspective, given it would be cached
etc. at that point, so possibly quicker. Thoughts? (I'm no expert on
spectre-v1 I'll admit).
Regards,
Phil
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
2023-06-15 23:31 ` Phillip Potter
@ 2023-06-16 3:14 ` Pawan Gupta
2023-06-16 9:39 ` Jordy Zomer
2023-06-17 9:40 ` Phillip Potter
0 siblings, 2 replies; 7+ messages in thread
From: Pawan Gupta @ 2023-06-16 3:14 UTC (permalink / raw
To: Phillip Potter; +Cc: linux-kernel, jordyzomer, linux-block
On Fri, Jun 16, 2023 at 12:31:50AM +0100, Phillip Potter wrote:
> I've now looked at this. It is possible for cdi->capacity to be > 1, as
> it is set via get_capabilities() -> cdrom_number_of_slots(), if the
> device is an individual or cartridge changer.
Ohk. Is there an upper limit to cdi->capacity? If not, we are left with
barrier_nospec().
> Therefore, I think using CDI_MAX_CAPACITY of 1 is not the correct
> approach. Jordy's V2 patch is fine therefore, but perhaps using
> array_index_nospec() with cdi->capacity is still better than a
> do/while loop from a performance perspective, given it would be cached
> etc. at that point, so possibly quicker. Thoughts? (I'm no expert on
> spectre-v1 I'll admit).
array_index_nospec() can only clip the arg correctly if the upper bound
is correct. Problem with array_index_nospec(arg, cdi->capacity) is
cdi->capacity is not a constant, so it suffers from the same problem as
arg i.e. cdi->capacity could also be speculated. Although having to
control 2 loads makes the attack difficult, but does not rules out
completely.
barrier_nospec() makes the CPU wait for all previous loads to retire
before executing following instructions speculatively. This causes the
conditional branch to resolve correctly. I hope this does not fall into
a hotpath.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
2023-06-16 3:14 ` Pawan Gupta
@ 2023-06-16 9:39 ` Jordy Zomer
2023-06-16 12:59 ` Randy Dunlap
2023-06-17 9:40 ` Phillip Potter
1 sibling, 1 reply; 7+ messages in thread
From: Jordy Zomer @ 2023-06-16 9:39 UTC (permalink / raw
To: Pawan Gupta; +Cc: Phillip Potter, linux-kernel, linux-block
Thanks for the explanation Pawan, a little bit off-topic for this patch but
shall I send a patch to add this to the documentation of array_index_nospec()
and fix other calls to that function where the upper bound is not a constant? :)
On Fri, Jun 16, 2023 at 5:15 AM Pawan Gupta
<pawan.kumar.gupta@linux.intel.com> wrote:
>
> On Fri, Jun 16, 2023 at 12:31:50AM +0100, Phillip Potter wrote:
> > I've now looked at this. It is possible for cdi->capacity to be > 1, as
> > it is set via get_capabilities() -> cdrom_number_of_slots(), if the
> > device is an individual or cartridge changer.
>
> Ohk. Is there an upper limit to cdi->capacity? If not, we are left with
> barrier_nospec().
>
> > Therefore, I think using CDI_MAX_CAPACITY of 1 is not the correct
> > approach. Jordy's V2 patch is fine therefore, but perhaps using
> > array_index_nospec() with cdi->capacity is still better than a
> > do/while loop from a performance perspective, given it would be cached
> > etc. at that point, so possibly quicker. Thoughts? (I'm no expert on
> > spectre-v1 I'll admit).
>
> array_index_nospec() can only clip the arg correctly if the upper bound
> is correct. Problem with array_index_nospec(arg, cdi->capacity) is
> cdi->capacity is not a constant, so it suffers from the same problem as
> arg i.e. cdi->capacity could also be speculated. Although having to
> control 2 loads makes the attack difficult, but does not rules out
> completely.
>
> barrier_nospec() makes the CPU wait for all previous loads to retire
> before executing following instructions speculatively. This causes the
> conditional branch to resolve correctly. I hope this does not fall into
> a hotpath.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
2023-06-16 9:39 ` Jordy Zomer
@ 2023-06-16 12:59 ` Randy Dunlap
0 siblings, 0 replies; 7+ messages in thread
From: Randy Dunlap @ 2023-06-16 12:59 UTC (permalink / raw
To: Jordy Zomer, Pawan Gupta; +Cc: Phillip Potter, linux-kernel, linux-block
On 6/16/23 02:39, Jordy Zomer wrote:
> Thanks for the explanation Pawan, a little bit off-topic for this patch but
> shall I send a patch to add this to the documentation of array_index_nospec()
> and fix other calls to that function where the upper bound is not a constant? :)
>
Yes, please. We don't want to lose that info.
Thanks.
> On Fri, Jun 16, 2023 at 5:15 AM Pawan Gupta
> <pawan.kumar.gupta@linux.intel.com> wrote:
>>
>> On Fri, Jun 16, 2023 at 12:31:50AM +0100, Phillip Potter wrote:
>>> I've now looked at this. It is possible for cdi->capacity to be > 1, as
>>> it is set via get_capabilities() -> cdrom_number_of_slots(), if the
>>> device is an individual or cartridge changer.
>>
>> Ohk. Is there an upper limit to cdi->capacity? If not, we are left with
>> barrier_nospec().
>>
>>> Therefore, I think using CDI_MAX_CAPACITY of 1 is not the correct
>>> approach. Jordy's V2 patch is fine therefore, but perhaps using
>>> array_index_nospec() with cdi->capacity is still better than a
>>> do/while loop from a performance perspective, given it would be cached
>>> etc. at that point, so possibly quicker. Thoughts? (I'm no expert on
>>> spectre-v1 I'll admit).
>>
>> array_index_nospec() can only clip the arg correctly if the upper bound
>> is correct. Problem with array_index_nospec(arg, cdi->capacity) is
>> cdi->capacity is not a constant, so it suffers from the same problem as
>> arg i.e. cdi->capacity could also be speculated. Although having to
>> control 2 loads makes the attack difficult, but does not rules out
>> completely.
>>
>> barrier_nospec() makes the CPU wait for all previous loads to retire
>> before executing following instructions speculatively. This causes the
>> conditional branch to resolve correctly. I hope this does not fall into
>> a hotpath.
--
~Randy
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
[not found] ` <20230612110040.849318-2-jordyzomer@google.com>
2023-06-15 8:12 ` [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget Phillip Potter
[not found] ` <20230615163125.td3aodpfwth5n4mc@desk>
@ 2023-06-17 9:37 ` Phillip Potter
2 siblings, 0 replies; 7+ messages in thread
From: Phillip Potter @ 2023-06-17 9:37 UTC (permalink / raw
To: Jordy Zomer; +Cc: linux-kernel, linux-block
On Mon, Jun 12, 2023 at 11:00:40AM +0000, Jordy Zomer wrote:
> This patch fixes a spectre-v1 gadget in cdrom.
> The gadget could be triggered by,
> speculatviely bypassing the cdi->capacity check.
>
> Signed-off-by: Jordy Zomer <jordyzomer@google.com>
> ---
> drivers/cdrom/cdrom.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 416f723a2dbb..ecf2b458c108 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -264,6 +264,7 @@
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> +#include <linux/nospec.h>
> #include <linux/slab.h>
> #include <linux/cdrom.h>
> #include <linux/sysctl.h>
> @@ -2329,6 +2330,9 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> if (arg >= cdi->capacity)
> return -EINVAL;
>
> + /* Prevent arg from speculatively bypassing the length check */
> + barrier_nospec();
> +
> info = kmalloc(sizeof(*info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
> --
> 2.41.0.162.gfafddb0af9-goog
>
Hi Jordy,
Looks good to me,
Reviewed-by: Phillip Potter <phil@philpotter.co.uk>
I will forward on for inclusion.
Regards,
Phil
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
2023-06-16 3:14 ` Pawan Gupta
2023-06-16 9:39 ` Jordy Zomer
@ 2023-06-17 9:40 ` Phillip Potter
1 sibling, 0 replies; 7+ messages in thread
From: Phillip Potter @ 2023-06-17 9:40 UTC (permalink / raw
To: Pawan Gupta; +Cc: linux-kernel, jordyzomer, linux-block
On Thu, Jun 15, 2023 at 08:14:47PM -0700, Pawan Gupta wrote:
> On Fri, Jun 16, 2023 at 12:31:50AM +0100, Phillip Potter wrote:
> > I've now looked at this. It is possible for cdi->capacity to be > 1, as
> > it is set via get_capabilities() -> cdrom_number_of_slots(), if the
> > device is an individual or cartridge changer.
>
> Ohk. Is there an upper limit to cdi->capacity? If not, we are left with
> barrier_nospec().
>
No, from the perspective of the codebase, this value is read from the
device and is therefore arbitrary in theory.
> > Therefore, I think using CDI_MAX_CAPACITY of 1 is not the correct
> > approach. Jordy's V2 patch is fine therefore, but perhaps using
> > array_index_nospec() with cdi->capacity is still better than a
> > do/while loop from a performance perspective, given it would be cached
> > etc. at that point, so possibly quicker. Thoughts? (I'm no expert on
> > spectre-v1 I'll admit).
>
> array_index_nospec() can only clip the arg correctly if the upper bound
> is correct. Problem with array_index_nospec(arg, cdi->capacity) is
> cdi->capacity is not a constant, so it suffers from the same problem as
> arg i.e. cdi->capacity could also be speculated. Although having to
> control 2 loads makes the attack difficult, but does not rules out
> completely.
>
> barrier_nospec() makes the CPU wait for all previous loads to retire
> before executing following instructions speculatively. This causes the
> conditional branch to resolve correctly. I hope this does not fall into
> a hotpath.
Thanks for the explanation. Based on this and the fact that particular
ioctl function isn't likely on a hugely hot path for most users, I have
approved the patch via another e-mail.
Regards,
Phil
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-17 9:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230612110040.849318-1-jordyzomer@google.com>
[not found] ` <20230612110040.849318-2-jordyzomer@google.com>
2023-06-15 8:12 ` [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget Phillip Potter
[not found] ` <20230615163125.td3aodpfwth5n4mc@desk>
2023-06-15 23:31 ` Phillip Potter
2023-06-16 3:14 ` Pawan Gupta
2023-06-16 9:39 ` Jordy Zomer
2023-06-16 12:59 ` Randy Dunlap
2023-06-17 9:40 ` Phillip Potter
2023-06-17 9:37 ` Phillip Potter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).