[PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Nitesh Shetty]

This fixes the smatch warning, "nvme_loop_create_ctrl() warn:
'opts->queue_size - 1' 18446744073709551615 can't fit into 65535
'ctrl->ctrl.sqsize'"
We don't need size_t for queue_size, u16 should serve the purpose,
as max size is limited to NVMF_DEF_QUEUE_SIZE(1024).

Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 drivers/nvme/host/fabrics.h | 2 +-
 drivers/nvme/host/fc.c      | 2 +-
 drivers/nvme/host/rdma.c    | 2 +-
 drivers/nvme/host/tcp.c     | 2 +-
 drivers/nvme/target/loop.c  | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index fbaee5a7be19..0b2e45faa3b9 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -125,7 +125,7 @@ struct nvmf_ctrl_options {
 	char			*trsvcid;
 	char			*host_traddr;
 	char			*host_iface;
-	size_t			queue_size;
+	u16			queue_size;
 	unsigned int		nr_io_queues;
 	unsigned int		reconnect_delay;
 	bool			discovery_nqn;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 9f9a3b35dc64..7f680443b3cf 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3159,7 +3159,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
 		dev_warn(ctrl->ctrl.device,
-			"queue_size %zu > ctrl maxcmd %u, reducing "
+			"queue_size %u > ctrl maxcmd %u, reducing "
 			"to maxcmd\n",
 			opts->queue_size, ctrl->ctrl.maxcmd);
 		opts->queue_size = ctrl->ctrl.maxcmd;
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6d178d555920..d0b7543f753e 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1025,7 +1025,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 
 	if (ctrl->ctrl.opts->queue_size > ctrl->ctrl.sqsize + 1) {
 		dev_warn(ctrl->ctrl.device,
-			"queue_size %zu > ctrl sqsize %u, clamping down\n",
+			"queue_size %u > ctrl sqsize %u, clamping down\n",
 			ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
 	}
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index d79811cfa0ce..d03f5921d344 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2193,7 +2193,7 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 
 	if (opts->queue_size > ctrl->sqsize + 1)
 		dev_warn(ctrl->device,
-			"queue_size %zu > ctrl sqsize %u, clamping down\n",
+			"queue_size %u > ctrl sqsize %u, clamping down\n",
 			opts->queue_size, ctrl->sqsize + 1);
 
 	if (ctrl->sqsize + 1 > ctrl->maxcmd) {
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 9cb434c58075..0416f41fad19 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -573,7 +573,7 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
 		dev_warn(ctrl->ctrl.device,
-			"queue_size %zu > ctrl maxcmd %u, clamping down\n",
+			"queue_size %u > ctrl maxcmd %u, clamping down\n",
 			opts->queue_size, ctrl->ctrl.maxcmd);
 		opts->queue_size = ctrl->ctrl.maxcmd;
 	}

base-commit: da59b416ba80e43afb2b642e0cee73739f4c6079
-- 
2.35.1.500.gb896f729e2

[PATCH 2/2] nvme: prevent potential spectre v1 gadget

[Nitesh Shetty]

This patch fixes the smatch warning, "nvmet_ns_ana_grpid_store() warn:
potential spectre issue 'nvmet_ana_group_enabled' [w] (local cap)"
Prevent the contents of kernel memory from being leaked to  user space
via speculative execution by using array_index_nospec.

Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 drivers/nvme/target/configfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e307a044b1a1..d937fe05129e 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -18,6 +18,7 @@
 #include <linux/nvme-keyring.h>
 #include <crypto/hash.h>
 #include <crypto/kpp.h>
+#include <linux/nospec.h>
 
 #include "nvmet.h"
 
@@ -621,6 +622,7 @@ static ssize_t nvmet_ns_ana_grpid_store(struct config_item *item,
 
 	down_write(&nvmet_ana_sem);
 	oldgrpid = ns->anagrpid;
+	newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS);
 	nvmet_ana_group_enabled[newgrpid]++;
 	ns->anagrpid = newgrpid;
 	nvmet_ana_group_enabled[oldgrpid]--;
@@ -1812,6 +1814,7 @@ static struct config_group *nvmet_ana_groups_make_group(
 	grp->grpid = grpid;
 
 	down_write(&nvmet_ana_sem);
+	grpid = array_index_nospec(grpid, NVMET_MAX_ANAGRPS);
 	nvmet_ana_group_enabled[grpid]++;
 	up_write(&nvmet_ana_sem);
 
-- 
2.35.1.500.gb896f729e2

Re: [PATCH 2/2] nvme: prevent potential spectre v1 gadget

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/2] nvme: prevent potential spectre v1 gadget

[Dan Carpenter]

On Tue, Nov 28, 2023 at 05:59:57PM +0530, Nitesh Shetty wrote:
> This patch fixes the smatch warning, "nvmet_ns_ana_grpid_store() warn:
> potential spectre issue 'nvmet_ana_group_enabled' [w] (local cap)"
> Prevent the contents of kernel memory from being leaked to  user space
> via speculative execution by using array_index_nospec.
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> ---

When it comes to adding array_index_nospec(), then I don't know enough
about it to comment.

regards,
dan carpenter

Re: [PATCH 2/2] nvme: prevent potential spectre v1 gadget

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On 29/11/23 12:27PM, Dan Carpenter wrote:
>On Tue, Nov 28, 2023 at 05:59:57PM +0530, Nitesh Shetty wrote:
>> This patch fixes the smatch warning, "nvmet_ns_ana_grpid_store() warn:
>> potential spectre issue 'nvmet_ana_group_enabled' [w] (local cap)"
>> Prevent the contents of kernel memory from being leaked to  user space
>> via speculative execution by using array_index_nospec.
>>
>> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
>> ---
>
>When it comes to adding array_index_nospec(), then I don't know enough
>about it to comment.
>
Upon smatch's warning, I referred to kernel documentation[1].
Using array_index_nospec should avoid memory leak.

Thank you,
Nitesh Shetty

[1] https://www.kernel.org/doc/Documentation/speculation.txt

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] nvme: prevent potential spectre v1 gadget

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] nvme: prevent potential spectre v1 gadget

[Keith Busch]

Thanks, applied to nvme-6.7.

Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Dan Carpenter]

On Tue, Nov 28, 2023 at 05:59:56PM +0530, Nitesh Shetty wrote:
> This fixes the smatch warning, "nvme_loop_create_ctrl() warn:
> 'opts->queue_size - 1' 18446744073709551615 can't fit into 65535
> 'ctrl->ctrl.sqsize'"
> We don't need size_t for queue_size, u16 should serve the purpose,
> as max size is limited to NVMF_DEF_QUEUE_SIZE(1024).
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>

Huh...  I'm sorry I wasn't necessarily aware that I had published this
Smatch warning.  I feel like it has a high rate of false positives.

Generally with Smatch warnings, I'm not going to try silence every
false positive.  And I had one complaint recently that I was too focused
on silencing false positives instead of discovering new bugs...

The other thing about static analysis is that static checker developers
want 0% false positives and kernel developers want 100% false positives.
I'm a kernel developer so in code that I have looked at the rate of
false positives is very close to 100%.  Only new code has valid
warnings.

Here is what this code looks like:

drivers/nvme/target/loop.c
   573          if (opts->queue_size > ctrl->ctrl.maxcmd) {
   574                  /* warn if maxcmd is lower than queue_size */
   575                  dev_warn(ctrl->ctrl.device,
   576                          "queue_size %zu > ctrl maxcmd %u, clamping down\n",
   577                          opts->queue_size, ctrl->ctrl.maxcmd);
   578                  opts->queue_size = ctrl->ctrl.maxcmd;
   579          }
   580          ctrl->ctrl.sqsize = opts->queue_size - 1;

Smatch thinks that opts->queue_size is a value that comes from the user
in the 16-47 range.  But the bug is that Smatch thinks that
ctrl->ctrl.maxcmd is zero.  16 is greater than zero so we do the
opts->queue_size = ctrl->ctrl.maxcmd; assignment.  Then zero minus one
is ULONG_MAX so that's a very high number.

Smatch is just wrong in this case.  Let me try figure out what went
wrong.  The ctrl->ctrl.maxcmd = 0 comes from:

	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);

It's supposed to get set to unknown in nvme_loop_configure_admin_queue().
The database has the correct data.

$ smdb.py return_states nvme_loop_configure_admin_queue | grep maxcmd
drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 229 |             0|       PARAM_SET |  0 |       $->ctrl.maxcmd |             0-u16max |
drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 231 | s32min-(-1),1-s32max|       PARAM_ADD |  0 |       $->ctrl.maxcmd |             0-u16max |

But the issue is that Smatch thinks that nvme_loop_configure_admin_queue()
always fails with -12.  The reason for that is because Smatch thinks
that ctrl->ctrl.ops is NULL but the function can only succeed when it's
non-NULL.

The ctrl->ops assignment happens in nvme_init_ctrl() and it should have
been easy to track.  I am not sure what went wrong there.  I'll take a
look at that and fix it.

regards,
dan carpenter

Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]

On 29/11/23 12:26PM, Dan Carpenter wrote:
>On Tue, Nov 28, 2023 at 05:59:56PM +0530, Nitesh Shetty wrote:
>> This fixes the smatch warning, "nvme_loop_create_ctrl() warn:
>> 'opts->queue_size - 1' 18446744073709551615 can't fit into 65535
>> 'ctrl->ctrl.sqsize'"
>> We don't need size_t for queue_size, u16 should serve the purpose,
>> as max size is limited to NVMF_DEF_QUEUE_SIZE(1024).
>>
>> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
>
>Huh...  I'm sorry I wasn't necessarily aware that I had published this
>Smatch warning.  I feel like it has a high rate of false positives.
>
>Generally with Smatch warnings, I'm not going to try silence every
>false positive.  And I had one complaint recently that I was too focused
>on silencing false positives instead of discovering new bugs...
>
>The other thing about static analysis is that static checker developers
>want 0% false positives and kernel developers want 100% false positives.
>I'm a kernel developer so in code that I have looked at the rate of
>false positives is very close to 100%.  Only new code has valid
>warnings.
>
>Here is what this code looks like:
>
>drivers/nvme/target/loop.c
>   573          if (opts->queue_size > ctrl->ctrl.maxcmd) {
>   574                  /* warn if maxcmd is lower than queue_size */
>   575                  dev_warn(ctrl->ctrl.device,
>   576                          "queue_size %zu > ctrl maxcmd %u, clamping down\n",
>   577                          opts->queue_size, ctrl->ctrl.maxcmd);
>   578                  opts->queue_size = ctrl->ctrl.maxcmd;
>   579          }
>   580          ctrl->ctrl.sqsize = opts->queue_size - 1;
>
>Smatch thinks that opts->queue_size is a value that comes from the user
>in the 16-47 range.  But the bug is that Smatch thinks that
>ctrl->ctrl.maxcmd is zero.  16 is greater than zero so we do the
>opts->queue_size = ctrl->ctrl.maxcmd; assignment.  Then zero minus one
>is ULONG_MAX so that's a very high number.
>
>Smatch is just wrong in this case.  Let me try figure out what went
>wrong.  The ctrl->ctrl.maxcmd = 0 comes from:
>
>	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>
>It's supposed to get set to unknown in nvme_loop_configure_admin_queue().
>The database has the correct data.
>
>$ smdb.py return_states nvme_loop_configure_admin_queue | grep maxcmd
>drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 229 |             0|       PARAM_SET |  0 |       $->ctrl.maxcmd |             0-u16max |
>drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 231 | s32min-(-1),1-s32max|       PARAM_ADD |  0 |       $->ctrl.maxcmd |             0-u16max |
>
>But the issue is that Smatch thinks that nvme_loop_configure_admin_queue()
>always fails with -12.  The reason for that is because Smatch thinks
>that ctrl->ctrl.ops is NULL but the function can only succeed when it's
>non-NULL.
>
>The ctrl->ops assignment happens in nvme_init_ctrl() and it should have
>been easy to track.  I am not sure what went wrong there.  I'll take a
>look at that and fix it.

Thank you for this insight.
I ran smatch on complete kernel using smatch's test_kernel.sh
I was unaware of this smbd.py option. I will explore this.
Meanwhile I feel this patch is still relevant, as it aligns opts
queue_size with size of ctrl queue_size.

Regards,
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Dan Carpenter]

On Wed, Nov 29, 2023 at 04:18:37PM +0530, Nitesh Shetty wrote:
> Thank you for this insight.
> I ran smatch on complete kernel using smatch's test_kernel.sh
> I was unaware of this smbd.py option. I will explore this.

The ./smatch_scripts/build_kernel_data.sh command creates the cross
function db.  It takes a while though.  And it's probably better to
run it a few times because every time you run it the call tree chains
get one call longer.  I run it every night against the latest
linux-next.

> Meanwhile I feel this patch is still relevant, as it aligns opts
> queue_size with size of ctrl queue_size.

I don't have an opinion on this.

regards,
dan carpenter

Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

On 30/11/23 10:07AM, Dan Carpenter wrote:
>On Wed, Nov 29, 2023 at 04:18:37PM +0530, Nitesh Shetty wrote:
>> Thank you for this insight.
>> I ran smatch on complete kernel using smatch's test_kernel.sh
>> I was unaware of this smbd.py option. I will explore this.
>
>The ./smatch_scripts/build_kernel_data.sh command creates the cross
>function db.  It takes a while though.  And it's probably better to
>run it a few times because every time you run it the call tree chains
>get one call longer.  I run it every night against the latest
>linux-next.
>
I was not aware of repeated runs and chaining, will use repeat runs next time.

Thank you,
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size

[Dan Carpenter]

On Wed, Nov 29, 2023 at 12:26:24PM +0300, Dan Carpenter wrote:
> The ctrl->ops assignment happens in nvme_init_ctrl() and it should have
> been easy to track.  I am not sure what went wrong there.  I'll take a
> look at that and fix it.

I suspect on your system you don't have the cross function DB built so
it's not aware what happens in nvme_init_ctrl() or the other function I
mentioned.

On my system there is too much data so it isn't parsed.
1) When there is too much data it should just mark ctrl->ops as unknown
2) I should try to consolidate the data.  I should just mark all of
   ctrl->ctrl_device as unknown instead of recording any of the struct
   members individually.  There might also be stuff that isn't used like
   ctrl->namespaces_rwsem internals.
3) On my system, I have 2187 bogus entries that say we removed an item
   from a list.

Probably any of these fixes would silence the false positive but it
would be better to do all three.

If you don't have the cross function database though, you're probably
out of luck.  There are always going to be more false positives if you
don't have the cross function information.

regards,
dan carpenter