From: Kees Cook <keescook@chromium.org>
To: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Erick Archer <erick.archer@outlook.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
Bjorn Helgaas <bhelgaas@google.com>,
Justin Stitt <justinstitt@google.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-hardening@vger.kernel.org
Subject: Re: [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
Date: Mon, 29 Apr 2024 13:13:21 -0700 [thread overview]
Message-ID: <202404291259.3A8EE11@keescook> (raw)
In-Reply-To: <yq17cgg58sp.fsf@ca-mkp.ca.oracle.com>
On Mon, Apr 29, 2024 at 02:31:19PM -0400, Martin K. Petersen wrote:
>
> Kees,
>
> >> This patch seems to be lost. Gustavo reviewed it on January 15, 2024
> >> but the patch has not been applied since.
> >
> > This looks correct to me. I can pick this up if no one else snags it?
>
> I guess my original reply didn't make it out, I don't see it in the
> archives.
>
> My objections were:
>
> 1. The original code is more readable to me than the proposed
> replacement.
I guess this is a style preference. I find the proposed easier to read.
It also removes lines while doing it. :)
> 2. The original code has worked since introduced in 2012. Nobody has
> touched it since, presumably it's fine.
The code itself is fine unless you have a 32-bit system with a malicious
card, so yeah, near zero risk.
> 3. I don't have the hardware and thus no way of validating the proposed
> changes.
This is kind of an ongoing tension we have between driver code and
refactoring efforts. And this isn't a case where we can show identical
binary output, since this actively adds overflow checking via kcalloc()
internals.
> So what is the benefit of me accepting this patch? We have had several
> regressions in these conversions. Had one just last week, almost
> identical in nature to the one at hand.
People are working through large piles of known "weak code patterns"
with the goal of reaching 0 instances in the kernel. Usually this is for
ongoing greater compiler flag coverage, but this particular one is
harder for the compiler to warn on, so it's from Coccinelle patterns.
> I am all for fixing code which is undergoing active use and development.
> But I really don't see the benefit of updating a legacy driver which
> hasn't seen updates in ages. Why risk introducing a regression?
I see a common pattern where "why risk introducing a regression?" gets
paired with "we can't test this code". I'm really not sure what to do
about this given how much the kernel is changing all the time.
In this particular case, I guess all I can say is that it is a trivially
correct change that uses a more robust API and more idiomatic allocation
sizeof()s (i.e. use the sizeof() of what is being allocated, not a
potentially disconnected struct name).
-Kees
--
Kees Cook
next prev parent reply other threads:[~2024-04-29 20:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-30 16:17 [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc() Erick Archer
2024-04-27 13:52 ` Erick Archer
2024-04-29 17:20 ` Kees Cook
2024-04-29 18:31 ` Martin K. Petersen
2024-04-29 20:13 ` Kees Cook [this message]
2024-04-30 1:54 ` Finn Thain
2024-05-01 14:39 ` James Bottomley
2024-05-02 0:47 ` Finn Thain
2024-05-11 11:18 ` Erick Archer
2024-05-11 15:10 ` Erick Archer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202404291259.3A8EE11@keescook \
--to=keescook@chromium.org \
--cc=bhelgaas@google.com \
--cc=erick.archer@outlook.com \
--cc=gustavoars@kernel.org \
--cc=jejb@linux.ibm.com \
--cc=justinstitt@google.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).