linux-kernel - Re: [PATCH 1/3] fsi: occ: Force sequence numbering per OCC

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: <CACPK8XcBHGsFu0VoNPutC8HYbLcf0WV-KWNixCdGXxWsf1PDVg@mail.gmail.com>
Date:   Wed, 21 Jul 2021 02:37:28 +0000
From:   Joel Stanley <joel@....id.au>
To:     Eddie James <eajames@...ux.ibm.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-hwmon@...r.kernel.org, linux-fsi@...ts.ozlabs.org,
        Guenter Roeck <linux@...ck-us.net>,
        Jean Delvare <jdelvare@...e.com>, Jeremy Kerr <jk@...abs.org>,
        Alistair Popple <alistair@...ple.id.au>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH 1/3] fsi: occ: Force sequence numbering per OCC

On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@...ux.ibm.com> wrote:
>
> Set and increment the sequence number during the submit operation.
> This prevents sequence number conflicts between different users of
> the interface. A sequence number conflict may result in a user
> getting an OCC response meant for a different command. Since the
> sequence number is now modified, the checksum must be calculated and
> set before submitting the command.
>
> Signed-off-by: Eddie James <eajames@...ux.ibm.com>

Reviewed-by: Joel Stanley <joel@....id.au>

> @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>                 return -EINVAL;
>         }
>
> +       /* Checksum the request, ignoring first byte (sequence number). */
> +       for (i = 1; i < req_len - 2; ++i)
> +               checksum += byte_request[i];
> +

This could go below, after you've got the sequence number, so the
checksumming all happens in the same spot?

The driver has become a bit of a maze, I can't tell how you're
deciding what goes in fsi_occ_submit vs occ_write vs occ_putsram. If
oyu have some ideas on how to simplify it then I would welcome those
changes.



>         mutex_lock(&occ->occ_lock);
>
> -       /* Extract the seq_no from the command (first byte) */
> -       seq_no = *(const u8 *)request;
> -       rc = occ_putsram(occ, request, req_len);
> +       /*
> +        * Get a sequence number and update the counter. Avoid a sequence
> +        * number of 0 which would pass the response check below even if the
> +        * OCC response is uninitialized. Any sequence number the user is
> +        * trying to send is overwritten since this function is the only common
> +        * interface to the OCC and therefore the only place we can guarantee
> +        * unique sequence numbers.
> +        */
> +       seq_no = occ->sequence_number++;
> +       if (!occ->sequence_number)
> +               occ->sequence_number = 1;
> +       checksum += seq_no;
> +
> +       rc = occ_putsram(occ, request, req_len, seq_no, checksum);
>         if (rc)
>                 goto done;