linux-kernel - Re: [PATCH v1 5/6] fpga: add PolarFire SoC Auto Update support

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: <59750d1a-de31-4e89-b8a9-d97ef66aa5f6@spud>
Date:   Sat, 4 Mar 2023 17:01:53 +0000
From:   Conor Dooley <conor@...nel.org>
To:     Xu Yilun <yilun.xu@...el.com>
Cc:     Conor Dooley <conor.dooley@...rochip.com>,
        Daire McNamara <daire.mcnamara@...rochip.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Moritz Fischer <mdf@...nel.org>, Wu Hao <hao.wu@...el.com>,
        Tom Rix <trix@...hat.com>, linux-riscv@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fpga@...r.kernel.org
Subject: Re: [PATCH v1 5/6] fpga: add PolarFire SoC Auto Update support

On Sun, Mar 05, 2023 at 12:38:00AM +0800, Xu Yilun wrote:
> On 2023-02-17 at 16:40:22 +0000, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@...rochip.com>
> > 
> > Add support for Auto Update reprogramming of the FPGA fabric on
> > PolarFire SoC.
> > 
> > Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> > ---
> >  drivers/fpga/Kconfig                 |   9 +
> >  drivers/fpga/Makefile                |   1 +
> >  drivers/fpga/microchip-auto-update.c | 495 +++++++++++++++++++++++++++
> >  3 files changed, 505 insertions(+)
> >  create mode 100644 drivers/fpga/microchip-auto-update.c
> > +	/*
> > +	 * To verify that Auto Update is possible, the "Query Security Service

> Why verify the possibility here, if Auto Update is not possible, the
> Auto Update device should not be populated, is it?

Good point, I'll check this in probe instead.

> > +	/*
> > +	 * Populate the image address and then zero out the next directory so
> > +	 * that the system controller doesn't complain if in "Single Image"
> > +	 * mode.
> > +	 */
> > +	memcpy(buffer + AUTO_UPDATE_UPGRADE_DIRECTORY, &image_address, AUTO_UPDATE_DIRECTORY_WIDTH);
> > +	memset(buffer + AUTO_UPDATE_BLANK_DIRECTORY, 0x0, AUTO_UPDATE_DIRECTORY_WIDTH);
> 
> I'm wondering why the image address should be written for every
> updating? Seems it is only related to the flash size, not related to
> the to-be-programmed bitstream.

Yah, it doesn't need to be. I'll check it against the expected value &
only write it if needed.

> > +	dev_info(priv->dev, "Running verification of Upgrade Image\n");
> > +	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> > +	if (ret | response->resp_status) {
> > +		dev_warn(priv->dev, "Verification of Upgrade Image failed!\n");
> > +		ret = ret ? ret : -EBADMSG;
> 
> If verification failed, what happens to the written flash? Auto roll
> back?

Nope, that should be left up to userspace to decide what to do. I've got
some improvement to do to the mailbox driver that sits behind
mpfs_blocking_transaction() that I thought was not allowed by the
mailbox framework, so should be able to report better errors for this in
the future.

> > +	}
> > +
> > +	dev_info(priv->dev, "Verification of Upgrade Image passed!\n");
> > +//	/*
> > +//	 * If the validation has passed, initiate Auto Update.
> > +//	 * This service has no command data and no response data. It overloads
> > +//	 * mbox_offset with the image index in the flash's SPI directory where
> > +//	 * the bitstream is located.
> > +//	 * Once we attempt Auto Update either:
> > +//	 * - it passes and the board reboots
> > +//	 * - it fails and the board reboots to recover
> > +//	 * - the system controller aborts and we exit "gracefully".
> > +//	 *   "gracefully" since there is no interrupt produced & it just times
> > +//	 *   out.
> > +//	 */
> > +//	response->resp_msg = response_msg;
> > +//	response->resp_size = AUTO_UPDATE_PROGRAM_RESP_SIZE;
> > +//	message->cmd_opcode = AUTO_UPDATE_PROGRAM_CMD_OPCODE;
> > +//	message->cmd_data_size = AUTO_UPDATE_PROGRAM_CMD_DATA_SIZE;
> > +//	message->response = response;
> > +//	message->cmd_data = AUTO_UPDATE_PROGRAM_CMD_DATA;
> > +//	message->mbox_offset = 0; //field is ignored
> > +//	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
> > +//
> > +//	dev_info(priv->dev, "Running Auto Update command\n");
> > +//	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> > +//	if (ret && ret != -ETIMEDOUT)
> > +//		goto out;
> > +//
> > +//	/* *remove this for auto update*
> > +//	 * This return 0 is dead code. Either the Auto Update will fail, or it will pass
> > +//	 * & the FPGA will be rebooted in which case mpfs_blocking_transaction()
> > +//	 * will never return and Linux will die.
> > +//	 */
> > +//	return 0;
> 
> Why comment out this code block?

It was meant to be removed & must have snuck back in a rebase. This is my
test code that initiates the update from Linux, rather than at reboot.

I'm going to take a look at Russ' driver before I submit another version
of this (and the underlying mailbox stuff also needs changes).

Thanks for taking a look,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)