Re: The Case for Rust (in any system)

On Thu, Sep 5, 2024 at 4:51 PM Steffen Nurpmeso <steffen@sdaoden.eu> wrote:

> sigh.  now i am back.
>
> ske-89@pkmab.se wrote in
>  <202409052313.aa18097@berenice.pkmab.se>:
>  |Alan Somers <asomers@freebsd.org> wrote:
>  |> In fact, of all the C bug fixes that I've been involved with (as
>  |> either author or reviewer) since May, about three quarters could've
>  |> been avoided just by using a better language.
>  |...
>  |> To summarize, here's the list of this week's security advisories, and
>  |> also some other recent C bug fixes of my own involvement:
>  |
>  |After checking several of these examples, I'm wondering what the code
>  |would have looked like in some "better language", where those bugs would
>  |have been avoided?
>
> Examples.  I find the absolute opposite after checking four.
> Ie, if you implement some SCSI command
>
>   -
>   -     /*
>   -      * struct scsi_sense_data, which is currently set to 256 bytes, is
>   -      * larger than the largest allowed value for the length field in
> the
>   -      * REQUEST SENSE CDB, which is 252 bytes as of SPC-4.
>   -      */
>   -     ctsio->kern_data_len = cdb->length;
>   -     ctsio->kern_total_len = cdb->length;
>   +     = ctsio->kern_total_len =
>   +         MIN(cdb->length, sizeof(*sense_ptr));
>
> This is a super clear logic error, that even says the comment,
> which did not care for security related impacts.  It came in as
> part of a tremendious super large patch "Add the CAM Target Layer
> (CTL)." (130f4520cba830cc6da47c9f871fed78710a4709) in 2012, 34000
> lines of code additions.  There were a couple of fixup commits.
> It seems to have been sponsored, but i have no idea of review or
> anything.  Compared to the WireGuard stuff, for example.
> Now i had to look more deeply why the commit says three bytes
> whereas the naive eye counts four.  (Maybe NUL terminated.)
>

So context is king here. It isn't at all clear to me this is a bug.
252 is the maximum for SPC-4, true. However, this code
generates the sense code (a sense buffer is created by a device
and sent to the host, and CTL implements a SCSI device). cdb->length
is the length the caller has already allocated. You don't want the kernel
to write
more than that many bytes into the sense buffer, nor do you want it copying
more than what's allocated into the kernel to send to the device (I'm not
exaclty
sure where this called from, or if it's all in the kernel copying it
around). Capping it at 256
likely is unnecessary, but acts as a good sanity value. But since you
give current line numbers, functions or files, I can't check to see
if that's the case or not. The vast majority of sense buffers are like
4 to 20 bytes, and are often described by a simple C structure. I've not
looked at the target mode size of CAM much (CTL), but I have written a
lot on the host side.

But without the rest of the code, it's hard to say if your obvious code
is a mistake on your part, or not. And the comparison to wireguard is
baseless speculation that you should not engage in without more knowledge
of the situation. Its rude and unprofessional, and not at all conducive to
a meaningful dialog. While the commit itself was rather large, it was for
an already completed work that could not have been easily broken up
into smaller bits, which is clear from the commit message.

The commit message also states clearly that CTL was written at/by Copan
for it's weird kinda sorta FreeBSD product (that part isn't in the commit
message,
but I know from talking to Justin, Scott and Ken) and then ported to stock
FreeBSD by Spectra Logic. Ken Merry did this work, and also the port (with
outers,
if I recall correctly, though not in the commit message). Ken has several
hundred
commits to the FreeBSD SCSI stack (CAM). It had been in production for 7
years
prior to its being included upstreamed to FreeBSD. This is quite different
than the
wireguard situation, even on its face. Ken (and Spectra Logic in general)
has a very
good reputation for being conservative in what they commit and for testing
extensively (often in shipping product for years, as was the case here).

So to throw around insults like you did is completely uncalled for. While I
have
more context (I've worked with Ken on and off for 20-odd years), that's not
needed to understand this. And it's a violation of at least the spirit of
the Code
of Conduct we have.

There's plenty to find in bad C code, but this example is a bad example
for you to cite. It's not at all clear to me this is a bug (though without
the full
context I can't go check to see if there's something we need to fix, or
it's just
a misunderstanding). And you certainly don't need to toss in gratuitous
insults
to make your point: if the code is bad, your point is made without them.

Warner