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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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:
---
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 416f723a2dbb..e1c4f969ffda 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 */
+ arg = array_index_nospec(arg, CDI_MAX_CAPACITY);
+
info = kmalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return -ENOMEM;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 12869e6d4ebd..62e163dc29cc 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -662,7 +662,7 @@ static int sr_probe(struct device *dev)
cd->cdi.ops = &sr_dops;
cd->cdi.handle = cd;
cd->cdi.mask = 0;
- cd->cdi.capacity = 1;
+ cd->cdi.capacity = CDI_MAX_CAPACITY;
sprintf(cd->cdi.name, "sr%d", minor);
sdev->sector_size = 2048; /* A guess, just in case */
@@ -882,7 +882,7 @@ static int get_capabilities(struct scsi_cd *cd)
(buffer[n + 6] >> 5) == mechtype_cartridge_changer)
cd->cdi.capacity =
cdrom_number_of_slots(&cd->cdi);
- if (cd->cdi.capacity <= 1)
+ if (cd->cdi.capacity <= CDI_MAX_CAPACITY)
/* not a changer */
cd->cdi.mask |= CDC_SELECT_DISC;
/*else I don't think it can close its tray
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 67caa909e3e6..51c046354275 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -29,6 +29,8 @@ struct packet_command
void *reserved[1];
};
+#define CDI_MAX_CAPACITY 1
+
/*
* _OLD will use PIO transfer on atapi devices, _BPC_* will use DMA
*/
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 <[email protected]>
> > ---
> > 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
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.
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
<[email protected]> 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.
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
> <[email protected]> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
I will forward on for inclusion.
Regards,
Phil
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