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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0cd6862e-8037-40b3-9fde-b7f10d66e31c@suswa.mountain>
Date:   Wed, 29 Nov 2023 12:26:24 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Nitesh Shetty <nj.shetty@...sung.com>
Cc:     Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        James Smart <james.smart@...adcom.com>,
        Chaitanya Kulkarni <kch@...dia.com>, error27@...il.com,
        gost.dev@...sung.com, nitheshshetty@...il.com,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] nvme: Update type from size_t to u16 for
 opts->queue_size

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@...sung.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