[PATCH] tftp: roll-over block counter to prevent data packets timeouts
Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression
when fetching files over TFTP whose size is bigger than 65535 * block size.
grub> linux /images/pxeboot/vmlinuz
grub> echo $?
0
grub> initrd /images/pxeboot/initrd.img
error: timeout reading '/images/pxeboot/initrd.img'.
grub> echo $?
28
It is caused by the block number counter being a 16-bit field, which leads
to a maximum file size of ((1 << 16) - 1) * block size. Because GRUB sets
the block size to 1024 octets (by using the TFTP Blocksize Option from RFC
2348 [0]), the maximum file size that can be transferred is 67107840 bytes.
The TFTP PROTOCOL (REVISION 2) RFC 1350 [1] does not mention what a client
should do when a file size is bigger than the maximum, but most TFTP hosts
support the block number counter to be rolled over. That is, acking a data
packet with a block number of 0 is taken as if the 65356th block was acked.
It was working before because the block counter roll-over was happening due
an overflow. But that got fixed by the mentioned commit, which led to the
regression when attempting to fetch files larger than the maximum size.
To allow TFTP file transfers of unlimited size again, re-introduce a block
counter roll-over so the data packets are acked preventing the timeouts.
[0]: https://tools.ietf.org/html/rfc2348
[1]: https://tools.ietf.org/html/rfc1350
Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
Suggested-by: Peter Jones
Signed-off-by: Javier Martinez Canillas
---
grub-core/net/tftp.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
index 644135caf46..ba90c2bc45a 100644
--- a/grub-core/net/tftp.c
+++ b/grub-core/net/tftp.c
@@ -29,6 +29,8 @@
GRUB_MOD_LICENSE ("GPLv3+");
+#define BLK_MASK ((1 << 16) - 1)
+
/* IP port for the MTFTP server used for Intel's PXE */
enum
{
@@ -183,8 +185,19 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__
((unused)),
return GRUB_ERR_NONE;
}
- /* Ack old/retransmitted block. */
- if (grub_be_to_cpu16 (tftph->u.data.block) < data->block + 1)
+ /*
+ * Ack old/retransmitted block.
+ *
+ * The block number is a 16-bit counter, thus the maximum file size that
+ * could be transfered is 65535 * block size. Most TFTP hosts support to
+ * roll-over the block counter to allow unlimited transfer file size.
+ *
+ * This behavior is not defined in the RFC 1350 [0] but is implemented by
+ * most TFTP clients and hosts.
+ *
+ * [0]: https://tools.ietf.org/html/rfc1350
+ */
+ if (grub_be_to_cpu16 (tftph->u.data.block) < ((data->block + 1) &
BLK_MASK))
ack (data, grub_be_to_cpu16 (tftph->u.data.block));
/* Ignore unexpected block. */
else if (grub_be_to_cpu16 (tftph->u.data.block) > data->block + 1)
--
2.28.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors
On Mon, Aug 31, 2020 at 01:43:36PM -0500, Glenn Washburn wrote: > I just noticed that I have a couple minor non-functional changes that I > think will make this patch a little better. I had been planning on > updating my original patch, but since you're picking this up, this is a > better place to update. Let me know if you'd like the updated version > of the patch instead of my inline comments below. Yeah, I guess it's easier if you just send the updated patch in response to my patch series. I'll make sure to pick it up then. Thanks in advance! Patrick signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] cryptodisk: Properly handle non-512 byte sized sectors.
By default, dm-crypt internally uses an IV that corresponds to 512-byte
sectors, even when a larger sector size is specified. What this means is
that when using a larger sector size, the IV is incremented every sector.
However, the amount the IV is incremented is the number of 512 byte blocks
in a sector (ie 8 for 4K sectors). Confusingly the IV does not corespond to
the number of, for example, 4K sectors. So each cipher block in the fifth
4K sector will be encrypted with an IV equal to 32, as opposed to 32-39 for
each sequential 512 byte block or an IV of 4 for each cipher block in the
sector.
There are some encryption utilities which do it the intuitive way and have
the IV equal to the sector number regardless of sector size (ie. the fifth
sector would have an IV of 4 for each cipher block). And this is supported
by dm-crypt with the iv_large_sectors option and also cryptsetup as of 2.3.3
with the --iv-large-sectors, though not with LUKS headers (only with --type
plain). However, support for this has not been included as grub does not
support plain devices right now.
One gotcha here is that the encrypted split keys are encrypted with a hard-
coded 512-byte sector size. So even if your data is encrypted with 4K sector
sizes, the split key encrypted area must be decrypted with a block size of
512 (ie the IV increments every 512 bytes). This made these changes less
aestetically pleasing than desired.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 44 -
grub-core/disk/luks.c | 5 +++--
grub-core/disk/luks2.c | 7 +-
include/grub/cryptodisk.h | 9 +++-
4 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 0b63b7d96..6319f3164 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -224,7 +224,8 @@ lrw_xor (const struct lrw_sector *sec,
static gcry_err_code_t
grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
grub_uint8_t * data, grub_size_t len,
- grub_disk_addr_t sector, int do_encrypt)
+ grub_disk_addr_t sector, grub_size_t log_sector_size,
+ int do_encrypt)
{
grub_size_t i;
gcry_err_code_t err;
@@ -237,12 +238,13 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
return (do_encrypt ? grub_crypto_ecb_encrypt (dev->cipher, data, data, len)
: grub_crypto_ecb_decrypt (dev->cipher, data, data, len));
- for (i = 0; i < len; i += (1U << dev->log_sector_size))
+ for (i = 0; i < len; i += (1U << log_sector_size))
{
grub_size_t sz = ((dev->cipher->cipher->blocksize
+ sizeof (grub_uint32_t) - 1)
/ sizeof (grub_uint32_t));
grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4];
+ grub_uint64_t iv_calc = 0;
if (dev->rekey)
{
@@ -270,7 +272,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
if (!ctx)
return GPG_ERR_OUT_OF_MEMORY;
- tmp = grub_cpu_to_le64 (sector << dev->log_sector_size);
+ tmp = grub_cpu_to_le64 (sector << log_sector_size);
dev->iv_hash->init (ctx);
dev->iv_hash->write (ctx, dev->iv_prefix, dev->iv_prefix_len);
dev->iv_hash->write (ctx, &tmp, sizeof (tmp));
@@ -281,14 +283,16 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
}
break;
case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
- iv[1] = grub_cpu_to_le32 (sector >> 32);
+ iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
+ iv[1] = grub_cpu_to_le32 (iv_calc >> 32);
/* FALLTHROUGH */
case GRUB_CRYPTODISK_MODE_IV_PLAIN:
- iv[0] = grub_cpu_to_le32 (sector & 0x);
+ iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
+ iv[0] = grub_cpu_to_le32 (iv_calc & 0x);
break;
case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
- iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size));
- iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
+ iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
+ iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
& 0x);
break;
case GRUB_CRYPTODISK_MODE_IV_BENBI:
@@ -311,10 +315,10 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
case GRUB_CRYPTODISK_MODE_CBC:
if (do_encrypt)
err = grub_crypto_cbc_encrypt (dev->cipher, data + i, data + i,
- (1U << dev->log_sector_size), iv);
+ (1U << log_sector_size), iv);
else
err = grub_crypto_cbc_decrypt (dev->cipher, data + i, data + i,
- (1U <<
Re: [PATCH] cryptodisk: Properly handle non-512 byte sized sectors.
The main difference with this patch is that sector_size is renamed to log_sector_size, grub has enough inaccurate or misleading names. Additionally, rename LOG_SECTOR_SIZE to LUKS_LOG_SECTOR_SIZE and CRYPT_LOG_SECTOR_SIZE to GRUB_CRYPTODISK_IV_LOG_SIZE and moved to cryptodisk.h. Also a comment was reworded for clarity. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
