| 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
| ||
|
Message-ID: <CACPK8Xe-amsBm=Q7pLgyMvdrxP4oCXB4YgxFuQJLvvmM+4g3NQ@mail.gmail.com>
Date: Mon, 21 Feb 2022 09:07:00 +0000
From: Joel Stanley <joel@....id.au>
To: Eddie James <eajames@...ux.ibm.com>
Cc: linux-fsi@...ts.ozlabs.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Jeremy Kerr <jk@...abs.org>,
Alistair Popple <alistair@...ple.id.au>
Subject: Re: [PATCH v2] fsi: occ: Improve response status checking
On Tue, 8 Feb 2022 at 15:22, Eddie James <eajames@...ux.ibm.com> wrote:
>
> If the driver sequence number coincidentally equals the previous
> command response sequence number, the driver may proceed with
> fetching the entire buffer before the OCC has processed the current
> command. To be sure the correct response is obtained, check the
> command type and also retry if any of the response parameters have
> changed when the rest of the buffer is fetched. Also initialize the
> driver with a random sequence number in order to reduce the chances
> of this happening.
>
> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
Reviewed-by: Joel Stanley <joel@....id.au>
I've applied this for v5.18.
> ---
> Changes since v1:
> - Refactor the retry into one loop
> - Add a comment about the pseudo-random number
>
> drivers/fsi/fsi-occ.c | 87 ++++++++++++++++++++++++++++---------------
> 1 file changed, 56 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index 7eaab1be0aa4..c9cc75fbdfb9 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -451,6 +451,14 @@ static int occ_trigger_attn(struct occ *occ)
> return rc;
> }
>
> +static bool fsi_occ_response_not_ready(struct occ_response *resp, u8 seq_no,
> + u8 cmd_type)
> +{
> + return resp->return_status == OCC_RESP_CMD_IN_PRG ||
> + resp->return_status == OCC_RESP_CRIT_INIT ||
> + resp->seq_no != seq_no || resp->cmd_type != cmd_type;
> +}
> +
> int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
> void *response, size_t *resp_len)
> {
> @@ -461,10 +469,11 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
> struct occ_response *resp = response;
> size_t user_resp_len = *resp_len;
> u8 seq_no;
> + u8 cmd_type;
> u16 checksum = 0;
> u16 resp_data_length;
> const u8 *byte_request = (const u8 *)request;
> - unsigned long start;
> + unsigned long end;
> int rc;
> size_t i;
>
> @@ -478,6 +487,8 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
> return -EINVAL;
> }
>
> + cmd_type = byte_request[1];
> +
> /* Checksum the request, ignoring first byte (sequence number). */
> for (i = 1; i < req_len - 2; ++i)
> checksum += byte_request[i];
> @@ -509,51 +520,61 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
> if (rc)
> goto done;
>
> - /* Read occ response header */
> - start = jiffies;
> - do {
> + end = jiffies + timeout;
> + while (true) {
> + /* Read occ response header */
> rc = occ_getsram(occ, 0, resp, 8);
> if (rc)
> goto done;
>
> - if (resp->return_status == OCC_RESP_CMD_IN_PRG ||
> - resp->return_status == OCC_RESP_CRIT_INIT ||
> - resp->seq_no != seq_no) {
> - rc = -ETIMEDOUT;
> -
> - if (time_after(jiffies, start + timeout)) {
> - dev_err(occ->dev, "resp timeout status=%02x "
> - "resp seq_no=%d our seq_no=%d\n",
> + if (fsi_occ_response_not_ready(resp, seq_no, cmd_type)) {
> + if (time_after(jiffies, end)) {
> + dev_err(occ->dev,
> + "resp timeout status=%02x seq=%d cmd=%d, our seq=%d cmd=%d\n",
> resp->return_status, resp->seq_no,
> - seq_no);
> + resp->cmd_type, seq_no, cmd_type);
> + rc = -ETIMEDOUT;
> goto done;
> }
>
> set_current_state(TASK_UNINTERRUPTIBLE);
> schedule_timeout(wait_time);
> - }
> - } while (rc);
> -
> - /* Extract size of response data */
> - resp_data_length = get_unaligned_be16(&resp->data_length);
> + } else {
> + /* Extract size of response data */
> + resp_data_length =
> + get_unaligned_be16(&resp->data_length);
> +
> + /*
> + * Message size is data length + 5 bytes header + 2
> + * bytes checksum
> + */
> + if ((resp_data_length + 7) > user_resp_len) {
> + rc = -EMSGSIZE;
> + goto done;
> + }
>
> - /* Message size is data length + 5 bytes header + 2 bytes checksum */
> - if ((resp_data_length + 7) > user_resp_len) {
> - rc = -EMSGSIZE;
> - goto done;
> + /*
> + * Get the entire response including the header again,
> + * in case it changed
> + */
> + if (resp_data_length > 1) {
> + rc = occ_getsram(occ, 0, resp,
> + resp_data_length + 7);
> + if (rc)
> + goto done;
> +
> + if (!fsi_occ_response_not_ready(resp, seq_no,
> + cmd_type))
> + break;
> + } else {
> + break;
> + }
> + }
> }
>
> dev_dbg(dev, "resp_status=%02x resp_data_len=%d\n",
> resp->return_status, resp_data_length);
>
> - /* Grab the rest */
> - if (resp_data_length > 1) {
> - /* already got 3 bytes resp, also need 2 bytes checksum */
> - rc = occ_getsram(occ, 8, &resp->data[3], resp_data_length - 1);
> - if (rc)
> - goto done;
> - }
> -
> occ->client_response_size = resp_data_length + 7;
> rc = occ_verify_checksum(occ, resp, resp_data_length);
>
> @@ -598,7 +619,11 @@ static int occ_probe(struct platform_device *pdev)
> occ->version = (uintptr_t)of_device_get_match_data(dev);
> occ->dev = dev;
> occ->sbefifo = dev->parent;
> - occ->sequence_number = 1;
> + /*
> + * Quickly derive a pseudo-random number from jiffies so that
> + * re-probing the driver doesn't accidentally overlap sequence numbers.
> + */
> + occ->sequence_number = (u8)((jiffies % 0xff) + 1);
> mutex_init(&occ->occ_lock);
>
> if (dev->of_node) {
> --
> 2.27.0
>
Powered by blists - more mailing lists