| Message ID | 20231024011531.442587-2-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extend struct tpm_buf to support sized buffers (TPM2B) | expand |
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Thanks I'll add it to the next round. For the tpm_buf_read(), I was thinking along the lines of: /** * tpm_buf_read() - Read from a TPM buffer * @buf: &tpm_buf instance * @pos: position within the buffer * @count: the number of bytes to read * @output: the output buffer * * Read bytes from a TPM buffer, and update the position. Returns false when the * amount of bytes requested would overflow the buffer, which is expected to * only happen in the case of hardware failure. */ static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output) { off_t next = *pos + count; if (next >= buf->length) { pr_warn("%s: %lu >= %lu\n", __func__, next, *offset); return false; } memcpy(output, &buf->data[*pos], count); *offset = next; return true; } BR, Jarkko
On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote: > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > Thanks I'll add it to the next round. > > For the tpm_buf_read(), I was thinking along the lines of: > > /** > * tpm_buf_read() - Read from a TPM buffer > * @buf: &tpm_buf instance > * @pos: position within the buffer > * @count: the number of bytes to read > * @output: the output buffer > * > * Read bytes from a TPM buffer, and update the position. Returns false when the > * amount of bytes requested would overflow the buffer, which is expected to > * only happen in the case of hardware failure. > */ > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output) > { > off_t next = *pos + count; > > if (next >= buf->length) { > pr_warn("%s: %lu >= %lu\n", __func__, next, *offset); > return false; > } > > memcpy(output, &buf->data[*pos], count); > *offset = next; > return true; > } > > BR, Jarkko > Then the callers will check, and return -EIO?
On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote: > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote: > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > Thanks I'll add it to the next round. > > > > For the tpm_buf_read(), I was thinking along the lines of: > > > > /** > > * tpm_buf_read() - Read from a TPM buffer > > * @buf: &tpm_buf instance > > * @pos: position within the buffer > > * @count: the number of bytes to read > > * @output: the output buffer > > * > > * Read bytes from a TPM buffer, and update the position. Returns > > false when the > > * amount of bytes requested would overflow the buffer, which is > > expected to > > * only happen in the case of hardware failure. > > */ > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, > > size_t count, void *output) > > { > > off_t next = *pos + count; > > > > if (next >= buf->length) { > > pr_warn("%s: %lu >= %lu\n", __func__, next, > > *offset); > > return false; > > } > > > > memcpy(output, &buf->data[*pos], count); > > *offset = next; > > return true; > > } > > > > BR, Jarkko > > > > Then the callers will check, and return -EIO? Really, no, why would we do that? The initial buffer is a page and no TPM currently can have a command that big, so if the buffer overflows, it's likely a programming error (failure to terminate loop or something) rather than a runtime one (a user actually induced a command that big and wanted it to be sent to the TPM). The only reason you might need to check is the no-alloc case and you passed in a much smaller buffer, but even there, I would guess it will come down to a coding fault not a possible runtime error. James
On Thu, Oct 26, 2023 at 01:55:55PM -0400, James Bottomley wrote: > On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote: > > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote: > > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > > > Thanks I'll add it to the next round. > > > > > > For the tpm_buf_read(), I was thinking along the lines of: > > > > > > /** > > > * tpm_buf_read() - Read from a TPM buffer > > > * @buf: &tpm_buf instance > > > * @pos: position within the buffer > > > * @count: the number of bytes to read > > > * @output: the output buffer > > > * > > > * Read bytes from a TPM buffer, and update the position. Returns > > > false when the > > > * amount of bytes requested would overflow the buffer, which is > > > expected to > > > * only happen in the case of hardware failure. > > > */ > > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, > > > size_t count, void *output) > > > { > > > off_t next = *pos + count; > > > > > > if (next >= buf->length) { > > > pr_warn("%s: %lu >= %lu\n", __func__, next, > > > *offset); > > > return false; > > > } > > > > > > memcpy(output, &buf->data[*pos], count); > > > *offset = next; > > > return true; > > > } > > > > > > BR, Jarkko > > > > > > > Then the callers will check, and return -EIO? > > Really, no, why would we do that? > > The initial buffer is a page and no TPM currently can have a command > that big, so if the buffer overflows, it's likely a programming error > (failure to terminate loop or something) rather than a runtime one (a > user actually induced a command that big and wanted it to be sent to > the TPM). The only reason you might need to check is the no-alloc case > and you passed in a much smaller buffer, but even there, I would guess > it will come down to a coding fault not a possible runtime error. > > James > I was clarifying based on this exchange below between Jarkko and Mario discussing patch 5/6. From https://lore.kernel.org/all/CWGM2YH00DJ3.JKSYNNEWVRW4@suppilovahvero/ : > > In the overflow case wouldn't you want to pass an error code up instead > > of just showing a WARN trace? > > > > The helper functions can't tell the difference, and the net outcome is > > going to be that if there is overflow you get a warning trace in the > > kernel log and whatever garbage "value" happened to have going to the > > caller. It's a programmer error but it's also unpredictable what > > happens here. > > > > I think it's cleaner to have callers of > > tpm_buf_read_u8/tpm_buf_read_u16/tpm_buf_read_u32 to to be able to know > > something wrong happened. > > I think you have a fair point here and I also think it is also a bigger > issue for the response parsing than programmer error. I.e. faulty or > malicious TPM could return corrupted data, which makes WARN() wrong > choice. > > So, as a corrective measure I think it should be pr_warn() instead, and > instead of returning u8/u16/u32, all functions should return 'ssize_t' > and -EIO in the case of overflow. > > Thank you, it was a really good catch. > > BR, Jarkko
On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote: > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote: > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > Thanks I'll add it to the next round. > > > > For the tpm_buf_read(), I was thinking along the lines of: > > > > /** > > * tpm_buf_read() - Read from a TPM buffer > > * @buf: &tpm_buf instance > > * @pos: position within the buffer > > * @count: the number of bytes to read > > * @output: the output buffer > > * > > * Read bytes from a TPM buffer, and update the position. Returns false when the > > * amount of bytes requested would overflow the buffer, which is expected to > > * only happen in the case of hardware failure. > > */ > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output) > > { > > off_t next = *pos + count; > > > > if (next >= buf->length) { > > pr_warn("%s: %lu >= %lu\n", __func__, next, *offset); > > return false; > > } > > > > memcpy(output, &buf->data[*pos], count); > > *offset = next; > > return true; > > } > > > > BR, Jarkko > > > > Then the callers will check, and return -EIO? Most likely. My thinking with boolean was that this way it sort of documents that this function can have only single possible error state. If it was int there could be possibly some other error codes to handle... Just would prefer this based on my experience calling other in-kernel functions. I often need to use lxr a lot just to get full understanding of all possible POSIX codes... BR, Jarkko
On Thu, 2023-10-26 at 13:55 -0400, James Bottomley wrote: > On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote: > > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote: > > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > > > Thanks I'll add it to the next round. > > > > > > For the tpm_buf_read(), I was thinking along the lines of: > > > > > > /** > > > * tpm_buf_read() - Read from a TPM buffer > > > * @buf: &tpm_buf instance > > > * @pos: position within the buffer > > > * @count: the number of bytes to read > > > * @output: the output buffer > > > * > > > * Read bytes from a TPM buffer, and update the position. Returns > > > false when the > > > * amount of bytes requested would overflow the buffer, which is > > > expected to > > > * only happen in the case of hardware failure. > > > */ > > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, > > > size_t count, void *output) > > > { > > > off_t next = *pos + count; > > > > > > if (next >= buf->length) { > > > pr_warn("%s: %lu >= %lu\n", __func__, next, > > > *offset); > > > return false; > > > } > > > > > > memcpy(output, &buf->data[*pos], count); > > > *offset = next; > > > return true; > > > } > > > > > > BR, Jarkko > > > > > > > Then the callers will check, and return -EIO? > > Really, no, why would we do that? > > The initial buffer is a page and no TPM currently can have a command > that big, so if the buffer overflows, it's likely a programming error > (failure to terminate loop or something) rather than a runtime one (a > user actually induced a command that big and wanted it to be sent to > the TPM). The only reason you might need to check is the no-alloc case > and you passed in a much smaller buffer, but even there, I would guess > it will come down to a coding fault not a possible runtime error. Yeah, this was my thinking too. So in HMAC case you anyway would not need to check it because crypto is destined to fail anyway. Returning boolean here does no harm so I thought that this is overally good compromise. BR, Jarkko
On Sun, 2023-11-05 at 23:59 +0200, Jarkko Sakkinen wrote: > On Thu, 2023-10-26 at 13:55 -0400, James Bottomley wrote: > > On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote: > > > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote: > > > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > > > > > > Thanks I'll add it to the next round. > > > > > > > > For the tpm_buf_read(), I was thinking along the lines of: > > > > > > > > /** > > > > * tpm_buf_read() - Read from a TPM buffer > > > > * @buf: &tpm_buf instance > > > > * @pos: position within the buffer > > > > * @count: the number of bytes to read > > > > * @output: the output buffer > > > > * > > > > * Read bytes from a TPM buffer, and update the position. Returns > > > > false when the > > > > * amount of bytes requested would overflow the buffer, which is > > > > expected to > > > > * only happen in the case of hardware failure. > > > > */ > > > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, > > > > size_t count, void *output) > > > > { > > > > off_t next = *pos + count; > > > > > > > > if (next >= buf->length) { > > > > pr_warn("%s: %lu >= %lu\n", __func__, next, > > > > *offset); > > > > return false; > > > > } > > > > > > > > memcpy(output, &buf->data[*pos], count); > > > > *offset = next; > > > > return true; > > > > } > > > > > > > > BR, Jarkko > > > > > > > > > > Then the callers will check, and return -EIO? > > > > Really, no, why would we do that? > > > > The initial buffer is a page and no TPM currently can have a command > > that big, so if the buffer overflows, it's likely a programming error > > (failure to terminate loop or something) rather than a runtime one (a > > user actually induced a command that big and wanted it to be sent to > > the TPM). The only reason you might need to check is the no-alloc case > > and you passed in a much smaller buffer, but even there, I would guess > > it will come down to a coding fault not a possible runtime error. > > > Yeah, this was my thinking too. So in HMAC case you anyway would not > need to check it because crypto is destined to fail anyway. > > Returning boolean here does no harm so I thought that this is overally > good compromise. Or actually maybe we should go just with void, as it does have even then "return value", as it emits to klog, right? BR, Jarkko
On Mon, 2023-11-06 at 00:01 +0200, Jarkko Sakkinen wrote: > On Sun, 2023-11-05 at 23:59 +0200, Jarkko Sakkinen wrote: > > On Thu, 2023-10-26 at 13:55 -0400, James Bottomley wrote: > > > On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote: > > > > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen > > > > wrote: > > > > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote: > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote: > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > > > > > > > > > Thanks I'll add it to the next round. > > > > > > > > > > For the tpm_buf_read(), I was thinking along the lines of: > > > > > > > > > > /** > > > > > * tpm_buf_read() - Read from a TPM buffer > > > > > * @buf: &tpm_buf instance > > > > > * @pos: position within the buffer > > > > > * @count: the number of bytes to read > > > > > * @output: the output buffer > > > > > * > > > > > * Read bytes from a TPM buffer, and update the position. > > > > > Returns > > > > > false when the > > > > > * amount of bytes requested would overflow the buffer, which > > > > > is > > > > > expected to > > > > > * only happen in the case of hardware failure. > > > > > */ > > > > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t > > > > > *pos, > > > > > size_t count, void *output) > > > > > { > > > > > off_t next = *pos + count; > > > > > > > > > > if (next >= buf->length) { > > > > > pr_warn("%s: %lu >= %lu\n", __func__, next, > > > > > *offset); > > > > > return false; > > > > > } > > > > > > > > > > memcpy(output, &buf->data[*pos], count); > > > > > *offset = next; > > > > > return true; > > > > > } > > > > > > > > > > BR, Jarkko > > > > > > > > > > > > > Then the callers will check, and return -EIO? > > > > > > Really, no, why would we do that? > > > > > > The initial buffer is a page and no TPM currently can have a > > > command that big, so if the buffer overflows, it's likely a > > > programming error (failure to terminate loop or something) rather > > > than a runtime one (a user actually induced a command that big > > > and wanted it to be sent to the TPM). The only reason you might > > > need to check is the no-alloc case and you passed in a much > > > smaller buffer, but even there, I would guess it will come down > > > to a coding fault not a possible runtime error. > > > > > > Yeah, this was my thinking too. So in HMAC case you anyway would > > not need to check it because crypto is destined to fail anyway. > > > > Returning boolean here does no harm so I thought that this is > > overally good compromise. > > Or actually maybe we should go just with void, as it does have even > then "return value", as it emits to klog, right? Right, these functions are almost drop in replacements for the get_inc_XX ones. The latter were void returning because all the knowledge of what is being looked for is in the calling routine, because it knows at a macro level what structure the buffer should have. James
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 0222b1ddb310..ad3594e383e1 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o tpm-y += eventlog/common.o tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o +tpm-y += tpm-buf.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c new file mode 100644 index 000000000000..88ce1a5402de --- /dev/null +++ b/drivers/char/tpm/tpm-buf.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Handing for tpm_buf structures to facilitate the building of commands + */ + +#include <linux/module.h> +#include <linux/tpm.h> + +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) +{ + buf->data = (u8 *)__get_free_page(GFP_KERNEL); + if (!buf->data) + return -ENOMEM; + + buf->flags = 0; + tpm_buf_reset(buf, tag, ordinal); + return 0; +} +EXPORT_SYMBOL_GPL(tpm_buf_init); + +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + + head->tag = cpu_to_be16(tag); + head->length = cpu_to_be32(sizeof(*head)); + head->ordinal = cpu_to_be32(ordinal); +} +EXPORT_SYMBOL_GPL(tpm_buf_reset); + +void tpm_buf_destroy(struct tpm_buf *buf) +{ + free_page((unsigned long)buf->data); +} +EXPORT_SYMBOL_GPL(tpm_buf_destroy); + +u32 tpm_buf_length(struct tpm_buf *buf) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + + return be32_to_cpu(head->length); +} +EXPORT_SYMBOL_GPL(tpm_buf_length); + +void tpm_buf_append(struct tpm_buf *buf, + const unsigned char *new_data, + unsigned int new_len) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + u32 len = tpm_buf_length(buf); + + /* Return silently if overflow has already happened. */ + if (buf->flags & TPM_BUF_OVERFLOW) + return; + + if ((len + new_len) > PAGE_SIZE) { + WARN(1, "tpm_buf: overflow\n"); + buf->flags |= TPM_BUF_OVERFLOW; + return; + } + + memcpy(&buf->data[len], new_data, new_len); + head->length = cpu_to_be32(len + new_len); +} +EXPORT_SYMBOL_GPL(tpm_buf_append); + +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value) +{ + tpm_buf_append(buf, &value, 1); +} +EXPORT_SYMBOL_GPL(tpm_buf_append_u8); + +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value) +{ + __be16 value2 = cpu_to_be16(value); + + tpm_buf_append(buf, (u8 *)&value2, 2); +} +EXPORT_SYMBOL_GPL(tpm_buf_append_u16); + +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) +{ + __be32 value2 = cpu_to_be32(value); + + tpm_buf_append(buf, (u8 *)&value2, 4); +} +EXPORT_SYMBOL_GPL(tpm_buf_append_u32); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 4ee9d13749ad..60032c60994b 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -326,84 +326,16 @@ struct tpm2_hash { unsigned int tpm_id; }; -static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - - head->tag = cpu_to_be16(tag); - head->length = cpu_to_be32(sizeof(*head)); - head->ordinal = cpu_to_be32(ordinal); -} - -static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) -{ - buf->data = (u8 *)__get_free_page(GFP_KERNEL); - if (!buf->data) - return -ENOMEM; - - buf->flags = 0; - tpm_buf_reset(buf, tag, ordinal); - return 0; -} - -static inline void tpm_buf_destroy(struct tpm_buf *buf) -{ - free_page((unsigned long)buf->data); -} - -static inline u32 tpm_buf_length(struct tpm_buf *buf) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - - return be32_to_cpu(head->length); -} - -static inline u16 tpm_buf_tag(struct tpm_buf *buf) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - - return be16_to_cpu(head->tag); -} - -static inline void tpm_buf_append(struct tpm_buf *buf, - const unsigned char *new_data, - unsigned int new_len) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - u32 len = tpm_buf_length(buf); - - /* Return silently if overflow has already happened. */ - if (buf->flags & TPM_BUF_OVERFLOW) - return; - - if ((len + new_len) > PAGE_SIZE) { - WARN(1, "tpm_buf: overflow\n"); - buf->flags |= TPM_BUF_OVERFLOW; - return; - } - memcpy(&buf->data[len], new_data, new_len); - head->length = cpu_to_be32(len + new_len); -} - -static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value) -{ - tpm_buf_append(buf, &value, 1); -} - -static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value) -{ - __be16 value2 = cpu_to_be16(value); - - tpm_buf_append(buf, (u8 *) &value2, 2); -} - -static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) -{ - __be32 value2 = cpu_to_be32(value); - - tpm_buf_append(buf, (u8 *) &value2, 4); -} +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal); +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal); +void tpm_buf_destroy(struct tpm_buf *buf); +u32 tpm_buf_length(struct tpm_buf *buf); +void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data, + unsigned int new_len); +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value); +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value); +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value); /* * Check if TPM device is in the firmware upgrade mode.