date:20200909

Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

2020-09-09 Thread Javier Martinez Canillas

Hello Daniel,

Thanks for the review.

On 9/7/20 9:36 PM, Daniel Kiper wrote:
> On Tue, Sep 01, 2020 at 02:30:35PM +0200, Javier Martinez Canillas wrote:
>> Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression
> 
> Please drop the quotes.
>

Sure, I can do that but I wonder why you don't want the quotes.
That's the convention used in many other projects.

[snip] 

>>
>> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
> 
> Please drop this line.
>

Same question here. I think is important information, specially for
downstream since they could allow people to decide whether they need
to backport this patch or not.

[snip] 

>> +   */
>> +  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)
> 
> I think the fix is incomplete. You should change the line above too.
>

True, thanks for pointing it out.
 
> However, why do not do "((grub_uint16_t) (data->block + 1))" instead of
> "& BLK_MASK" in general?
> 

Indeed. I'll change that and post a v2.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

2020-09-09 Thread Daniel Kiper

Hey Javier,

On Wed, Sep 09, 2020 at 12:47:20PM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> Thanks for the review.
>
> On 9/7/20 9:36 PM, Daniel Kiper wrote:
> > On Tue, Sep 01, 2020 at 02:30:35PM +0200, Javier Martinez Canillas wrote:
> >> Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression
> >
> > Please drop the quotes.
> >
>
> Sure, I can do that but I wonder why you don't want the quotes.
> That's the convention used in many other projects.

I think quotes are superfluous if you have parentheses.

> [snip]
>
> >>
> >> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
> >
> > Please drop this line.
> >
>
> Same question here. I think is important information, specially for
> downstream since they could allow people to decide whether they need
> to backport this patch or not.

You duplicate the information which is above. Additionally, IMO "Fixes:"
should contain bug number, CVE number, link to the bug, etc. not the
commit id.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors

2020-09-09 Thread Daniel Kiper

On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote:
> From: Glenn Washburn 
>
> 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

s/32-39/32-9/?

> 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 
> Reviewed-by: Patrick Steinhardt 
> ---
>  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);

Why 32? Could you use a constant or add a comment here?

> /* 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));

Ditto?

> -   iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> +   iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));

Ditto?

[...]

> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 59702067a..2e1347b13 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -124,7 +124,7 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>return NULL;
>newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
>newdev->source_disk

Re: [PATCH v3 0/9] Cryptodisk fixes for v2.06

2020-09-09 Thread Daniel Kiper

Hey Patrick,

On Mon, Sep 07, 2020 at 05:27:27PM +0200, Patrick Steinhardt wrote:
> Hi,
>
> this is the third version of this patchset, collecting various fixes for
> LUKS2/cryptodisk for the upcoming release of GRUB v2.06.
>
> Besides my Reviewed-by tag, the only thing that changed is the final
> patch by Glenn. Quoting him:
>
> > 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.

A few patches require some polishing. However, feel free to add
  Reviewed-by: Daniel Kiper 
to the patches 1, 2, 5, 6 and 7.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] remove unneccessary trailing semicolon

2020-09-09 Thread Daniel Kiper

Hi Florian,

Your patch is missing
  Signed-off-by: Florian La Roche 

If you are OK with me adding it for you I will take this patch.

On Tue, Sep 01, 2020 at 07:11:59AM +0200, Florian La Roche via Grub-devel wrote:
> ---
>  util/grub.d/41_custom.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/grub.d/41_custom.in b/util/grub.d/41_custom.in
> index fcc21a987..a08363da1 100644
> --- a/util/grub.d/41_custom.in
> +++ b/util/grub.d/41_custom.in
> @@ -3,7 +3,7 @@ cat <  if [ -f  \${config_directory}/custom.cfg ]; then
>source \${config_directory}/custom.cfg
>  elif [ -z "\${config_directory}" -a -f  \$prefix/custom.cfg ]; then
> -  source \$prefix/custom.cfg;
> +  source \$prefix/custom.cfg
>  fi
>  EOF

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] remove unneccessary trailing semicolon

2020-09-09 Thread Florian La Roche via Grub-devel

Hello Daniel Kiper,



Daniel Kiper  schrieb am Mi., 9. Sep. 2020, 14:37:

> Hi Florian,
>
> Your patch is missing
>   Signed-off-by: Florian La Roche 
>
> If you are OK with me adding it for you I will take this patch.
>

OK.


Best regards,

Florian La Roche
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 0/4] Fix Travis CI

2020-09-09 Thread Petr Vorel

Hi,

although grub does not use Travis CI (or at least not publicly) some
developers might welcome fixed .travis.yml for testing their patches.

I haven't figured out how to fix riscv32 and mips.

Kind regards,
Petr

Petr Vorel (4):
  travis: Run bootstrap to fix build
  travis: Fix sparc64 build
  travis: Remove mips builds
  travis: Remove riscv32 build

 .travis.yml | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
2.27.0.rc0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 3/4] travis: Remove mips builds

2020-09-09 Thread Petr Vorel

Remove mips builds fix configure error:
configure: error: could not force big-endian)

Signed-off-by: Petr Vorel 
---
 .travis.yml | 4 
 1 file changed, 4 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index d019a0172..ca2a1fa5a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -87,10 +87,6 @@ matrix:
   env:
 - GRUB_TARGETS="ia64-efi"
 - CROSS_TARGETS="ia64-linux"
-- name: "mips"
-  env:
-- GRUB_TARGETS="mips-arc mipsel-arc mipsel-qemu_mips mips-qemu_mips"
-- CROSS_TARGETS="mips64-linux"
 - name: "arm"
   env:
 - GRUB_TARGETS="arm-coreboot arm-efi arm-uboot"
-- 
2.27.0.rc0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/4] travis: Fix sparc64 build

2020-09-09 Thread Petr Vorel

Instead non-existing sparc64-ieee1275 format use all 3 sparc64 formats:
sparc64-ieee1275-{aout,cdcore,raw}.

Signed-off-by: Petr Vorel 
---
 .travis.yml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 4bd05a30a..d019a0172 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -40,8 +40,8 @@ script:
 
   # Build all selected GRUB targets.
   - for target in $GRUB_TARGETS; do
-  plat=${target#*-};
-  arch=${target%-*};
+  arch=$(echo $target | cut -d- -f1);
+  plat=$(echo $target | cut -d- -f2);
   [ "$arch" = "arm64" ] && arch=aarch64-linux;
   [ "$arch" = "arm" ] && arch=arm-linux-gnueabi;
   [ "$arch" = "ia64" ] && arch=ia64-linux;
@@ -81,7 +81,7 @@ matrix:
 - CROSS_TARGETS="powerpc64-linux"
 - name: "sparc64"
   env:
-- GRUB_TARGETS="sparc64-ieee1275"
+- GRUB_TARGETS="sparc64-ieee1275-aout sparc64-ieee1275-cdcore 
sparc64-ieee1275-raw"
 - CROSS_TARGETS="sparc64-linux"
 - name: "ia64"
   env:
-- 
2.27.0.rc0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/4] travis: Run bootstrap to fix build

2020-09-09 Thread Petr Vorel

autogen.sh isn't enough:
$ ./autogen.sh
Gnulib not yet bootstrapped; run ./bootstrap instead.
The command "./autogen.sh" exited with 1.

Using bootstrap requires to install autopoint package.

Signed-off-by: Petr Vorel 
---
 .travis.yml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 81de20fa3..4bd05a30a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -12,6 +12,7 @@ language: c
 addons:
   apt:
 packages:
+- autopoint
 - libsdl1.2-dev
 - lzop
 - ovmf
@@ -35,7 +36,7 @@ before_script:
 script:
   # Comments must be outside the command strings below, or the Travis parser
   # will get confused.
-  - ./autogen.sh
+  - ./bootstrap
 
   # Build all selected GRUB targets.
   - for target in $GRUB_TARGETS; do
-- 
2.27.0.rc0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 4/4] travis: Remove riscv32 build

2020-09-09 Thread Petr Vorel

To fix travis error:
grub-mkimage: error: target 1036 not reachable from pc=ba.

Signed-off-by: Petr Vorel 
---
 .travis.yml | 4 
 1 file changed, 4 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index ca2a1fa5a..8628c526f 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -95,10 +95,6 @@ matrix:
   env:
 - GRUB_TARGETS="arm64-efi"
 - CROSS_TARGETS="aarch64-linux"
-- name: "riscv32"
-  env:
-- GRUB_TARGETS="riscv32-efi"
-- CROSS_TARGETS="riscv32-linux"
 - name: "riscv64"
   env:
 - GRUB_TARGETS="riscv64-efi"
-- 
2.27.0.rc0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

2020-09-09 Thread Alexey Makhalov


> On Sep 9, 2020, at 4:12 AM, Daniel Kiper  wrote:
> 
> Hey Javier,
> 
> On Wed, Sep 09, 2020 at 12:47:20PM +0200, Javier Martinez Canillas wrote:
>> Hello Daniel,
>> 
>> Thanks for the review.
>> 
>> On 9/7/20 9:36 PM, Daniel Kiper wrote:
>>> On Tue, Sep 01, 2020 at 02:30:35PM +0200, Javier Martinez Canillas wrote:
 Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression
>>> 
>>> Please drop the quotes.
>>> 
>> 
>> Sure, I can do that but I wonder why you don't want the quotes.
>> That's the convention used in many other projects.
> 
> I think quotes are superfluous if you have parentheses.
> 
>> [snip]
>> 
 
 Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
>>> 
>>> Please drop this line.
>>> 
>> 
>> Same question here. I think is important information, specially for
>> downstream since they could allow people to decide whether they need
>> to backport this patch or not.
> 
> You duplicate the information which is above. Additionally, IMO "Fixes:"
> should contain bug number, CVE number, link to the bug, etc. not the
> commit id.

I think “Fixes: commit id” should remain in place. It provides direct 
information
from what commit the bug existed in case of regression.

—Alexey



signature.asc
Description: Message signed with OpenPGP
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors

Re: [PATCH v3 0/9] Cryptodisk fixes for v2.06

Re: [PATCH] remove unneccessary trailing semicolon

Re: [PATCH] remove unneccessary trailing semicolon

[PATCH 0/4] Fix Travis CI

[PATCH 3/4] travis: Remove mips builds

[PATCH 2/4] travis: Fix sparc64 build

[PATCH 1/4] travis: Run bootstrap to fix build

[PATCH 4/4] travis: Remove riscv32 build

Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

12 matches

Site Navigation

Mail list logo

Footer information