* 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
[parent not found: <20230615163125.td3aodpfwth5n4mc@desk>]
* 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 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
* 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
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