[PATCH] Initialize local relocator subchunk struct to all zeros
The way the code is written the tofree variable would never be
passed to the free_subchunk() function uninitialized. Coverity
cannot determine this and flags the situation as "Using uninitialized
value...". The fix is just to initialize the local struct.
Fixes: CID 314016
Signed-off-by: Ross Philipson
---
grub-core/lib/relocator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/lib/relocator.c b/grub-core/lib/relocator.c
index 68ef128..bfcc70d 100644
--- a/grub-core/lib/relocator.c
+++ b/grub-core/lib/relocator.c
@@ -989,7 +989,7 @@ malloc_in_range (struct grub_relocator *rel,
if (j != 0 && events[j - 1].pos != events[j].pos)
{
grub_addr_t alloc_start, alloc_end;
- struct grub_relocator_subchunk tofree;
+ struct grub_relocator_subchunk tofree = {0};
struct grub_relocator_subchunk *curschu = &tofree;
if (!oom)
curschu = &res->subchunks[cural];
--
1.8.3.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Initialize local relocator subchunk struct to all zeros
Hi Ross,
This looks good to me.
On Thursday, 2022-07-14 at 09:41:28 -04, Ross Philipson wrote:
> The way the code is written the tofree variable would never be
> passed to the free_subchunk() function uninitialized. Coverity
> cannot determine this and flags the situation as "Using uninitialized
> value...". The fix is just to initialize the local struct.
>
> Fixes: CID 314016
>
> Signed-off-by: Ross Philipson
Reviewed-by: Darren Kenny
Thanks,
Darren.
> ---
> grub-core/lib/relocator.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/lib/relocator.c b/grub-core/lib/relocator.c
> index 68ef128..bfcc70d 100644
> --- a/grub-core/lib/relocator.c
> +++ b/grub-core/lib/relocator.c
> @@ -989,7 +989,7 @@ malloc_in_range (struct grub_relocator *rel,
> if (j != 0 && events[j - 1].pos != events[j].pos)
> {
> grub_addr_t alloc_start, alloc_end;
> - struct grub_relocator_subchunk tofree;
> + struct grub_relocator_subchunk tofree = {0};
> struct grub_relocator_subchunk *curschu = &tofree;
> if (!oom)
> curschu = &res->subchunks[cural];
> --
> 1.8.3.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 1/1] plainmount: Support plain encryption mode
On Mon, 11 Jul 2022 19:35:47 +
Maxim Fomin wrote:
> --- Original Message ---
> On Sunday, July 10th, 2022 at 9:07 PM, Glenn Washburn
> wrote:
>
> > > +
> > > +plainmount hd0,gpt1 -o 1048576
> > > +
> > > +
> > > +both create virtual devices with 1MiB offset on top of the specified
> > > partition. The
> > > +option @option{-o} is useful to specify offset which is not aligned to
> > > 512 byte
> >
> >
> > I just noticed that the code takes this parameter and turns it into a
> > number of sectors that the data is offset from the start of the device.
> > Thus internally this gets rounded down to be sector aligned anyway. In
> > fact, its aligned to the size of the sector specified with the -S
> > option. So we should get rid of the -o entirely. In fact, -o will not
> > work if it is not a multiple of the sector size specified with -S.
>
> Option -o is taken as a byte number, not as a sector number, it is not
> multiplied
> by 512 or by sector size. However later, indeed it is truncated to a sector
> size,
> loosing its meaning. I agree with the rest of the comment, this option is
> irrelevant
> and should be removed.
I never did say above that the -o value gets multiplied, it gets
aligned. Anyway, I think we agree here.
>
> >
> > > +sector size. Note: current cryptsetup implementation of plain mode and
> > > LUKS v1 restricts
> > > +alignment to 512 byte sector size. Specifying arbitrary byte alignment
> > > is useful only to
> > > +open LUKS v2 volume if master key is known or to open the volume
> > > encrypted by other
> > > +cryptographic implementation. Note: in LUKS revealing master key is not
> > > recommended
> > > +because it allows to open encrypted device directly bypassing the header
> > > and LUKS
> > > +security features.
> >
> >
> > These "Notes" should probably be reorganized as @footnote{Put note
> > inside of this.} You have two notes here, so perhaps two foot notes,
> > although it looks like here you have a nested note. I'm not sure if you
> > can have nested footnotes.
>
> I can change them to be just a single note.
>
> > > +
> > > +Password can be specified in two ways: as a password data from a keyfile
> > > or as a secret
> > > +passphrase. The keyfile parameter @option{-d} specifies path to the
> > > keyfile containing
> > > +password data and has higher priority than the other password method.
> > > Specified password
> > > +data in this mode is not hashed. The option @option{-O} specifies offset
> > > in terms of bytes
> >
> >
> > Hmm its not hashed? I thought a keyfile that contains 4 characters
> > "pass" would be the same as entering "pass" as the secret passphrase.
> > If the keyfile is not hashed, then the derived master key is different.
> > Are you Am I mistaken or missing something?
>
> I have taken this behavior from cryptsetup. Having binary key file data
> directly allows
> to have arbitrary password (not necessarily typable with a keyboard). Such
> feature is not
> very popular, but it exists. This feature is connected with use cases where
> user types
> password in non-English keyboard (possibly, a letter which is not easily
> encoded in UTF,
> or which encoding depends on keyboard layout), which cannot be typed at
> another computer.
Ok, yes, I was mistaken. Perhaps we should say after "not hashed", ",
and and will be used as the cipher key."
>
> > > +of the password data from the start of the keyfile. This option has no
> > > effect without the
> > > +option @option{-d}. Offset of password data in the keyfile can be
> > > specified directly with
> > > +option @option{-d} and GRUB blocklist syntax (for example: '-d
> > > (hd0,gpt1)2048+').
> >
> >
> > Should this be -O instead of -d?
>
> Why? The last part of quoted text says that alternative to option -O is the
> blocklist syntax
> which is part of the -d option.
Ok, I got confused by the wording. I think it should use "using" instead
of "and", eg. "Offset of password data in the keyfile can be specified
directly with option @option{-d} using GRUB blocklist syntax"
>
> > > +configured when creating the encrypted volume. Attempting to decrypt
> > > such volume with
> > > +different sector size than it was created with will not result in an
> > > error, but will
> > > +decrypt to random bytes and thus prevent accessing the volume.
> >
> >
> > Actually, some sectors will be decrypted correctly. So perhaps we
> > should say "but not all sectors will be properly decrypted, generally
> > causing the volume to be inaccessible"
>
> I would not complicate explanation. The difference between decrypted in such
> 'strange'
> scenario FS and normal FS is so large, that no fs will work. For example, I
> have a BTRFS
> on top of -S 4096 encrypted partition. Once on live system (not GRUB) I
> forgot to type
> -S 4096 and received internal BTRFS error in dmesg saying something about bad
> internal
> metadata state and then general mount error. So yes, BTRFS can recognize that
[PATCH v3 1/1] i386: Make pmtimer tsc calibration not take 51 seconds to fail
From: Peter Jones
On my laptop running at 2.4GHz, if I run a VM where tsc calibration
using pmtimer will fail presuming a broken pmtimer, it takes ~51 seconds
to do so (as measured with the stopwatch on my phone), with a tsc delta
of 0x1cd1c85300, or around 125 billion cycles.
If instead of trying to wait for 5-200ms to show up on the pmtimer, we try
to wait for 5-200us, it decides it's broken in ~0x2626aa0 TSCs, aka ~2.4
million cycles, or more or less instantly.
Additionally, this reading the pmtimer was returning 0x anyway,
and that's obviously an invalid return. I've added a check for that and
0 so we don't bother waiting for the test if what we're seeing is dead
pins with no response at all.
If "debug" includes "pmtimer", you will see one of the following three
outcomes. If pmtimer gives all 0 or all 1 bits, you will see:
pmtimer: 0xff bad_reads: 1
pmtimer: 0xff bad_reads: 2
pmtimer: 0xff bad_reads: 3
pmtimer: 0xff bad_reads: 4
pmtimer: 0xff bad_reads: 5
pmtimer: 0xff bad_reads: 6
pmtimer: 0xff bad_reads: 7
pmtimer: 0xff bad_reads: 8
pmtimer: 0xff bad_reads: 9
pmtimer: 0xff bad_reads: 10
timer is broken; giving up.
This outcome was tested using qemu+kvm with UEFI (OVMF) firmware and
these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX
If pmtimer gives any other bit patterns but is not actually marching
forward fast enough to use for clock calibration, you will see:
pmtimer delta is 0x0 (1904 iterations)
tsc delta is implausible: 0x2626aa0
This outcome was tested using grub compiled with
GRUB_PMTIMER_IGNORE_BAD_READS defined (so as not to trip the bad read
test) using qemu+kvm with UEFI (OVMF) firmware, and these options:
-machine pc-q35-2.10 -cpu Broadwell-noTSX
If pmtimer actually works, you'll see something like:
pmtimer delta is 0xdff
tsc delta is 0x278756
This outcome was tested using qemu+kvm with UEFI (OVMF) firmware, and
these options: -machine pc-i440fx-2.4 -cpu Broadwell-noTSX
I've also tested this outcome on a real Intel Xeon E3-1275v3 on an Intel
Server Board S1200V3RPS using the SDV.RP.B8 "Release" build here:
https://firmware.intel.com/sites/default/files/UEFIDevKit_S1200RP_vB8.zip
Signed-off-by: Peter Jones
Signed-off-by: Javier Martinez Canillas
Signed-off-by: Robbie Harwood
---
grub-core/kern/i386/tsc_pmtimer.c | 112 --
1 file changed, 92 insertions(+), 20 deletions(-)
diff --git a/grub-core/kern/i386/tsc_pmtimer.c
b/grub-core/kern/i386/tsc_pmtimer.c
index c9c3616997..d9b3765b01 100644
--- a/grub-core/kern/i386/tsc_pmtimer.c
+++ b/grub-core/kern/i386/tsc_pmtimer.c
@@ -28,40 +28,104 @@
#include
#include
+/*
+ * Define GRUB_PMTIMER_IGNORE_BAD_READS if you're trying to test a timer that's
+ * present but doesn't keep time well.
+ */
+// #define GRUB_PMTIMER_IGNORE_BAD_READS
+
grub_uint64_t
grub_pmtimer_wait_count_tsc (grub_port_t pmtimer,
grub_uint16_t num_pm_ticks)
{
grub_uint32_t start;
- grub_uint32_t last;
- grub_uint32_t cur, end;
+ grub_uint64_t cur, end;
grub_uint64_t start_tsc;
grub_uint64_t end_tsc;
- int num_iter = 0;
+ unsigned int num_iter = 0;
+#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
+ int bad_reads = 0;
+#endif
- start = grub_inl (pmtimer) & 0xff;
- last = start;
+ /*
+ * Some timers are 24-bit and some are 32-bit, but it doesn't make much
+ * difference to us. Caring which one we have isn't really worth it since
+ * the low-order digits will give us enough data to calibrate TSC. So just
+ * mask the top-order byte off.
+ */
+ cur = start = grub_inl (pmtimer) & 0x00ffUL;
end = start + num_pm_ticks;
start_tsc = grub_get_tsc ();
while (1)
{
- cur = grub_inl (pmtimer) & 0xff;
- if (cur < last)
- cur |= 0x100;
- num_iter++;
+ cur &= 0xff00ULL;
+ /*
+ * Only take the low-order 24-bit for the reason explained above.
+ */
+ cur |= grub_inl (pmtimer) & 0x00ffUL;
+
+ end_tsc = grub_get_tsc();
+
+#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
+ /*
+ * If we get 10 reads in a row that are obviously dead pins, there's no
+ * reason to do this thousands of times.
+ */
+ if (cur == 0xffUL || cur == 0)
+ {
+ bad_reads++;
+ grub_dprintf ("pmtimer",
+ "pmtimer: 0x%"PRIxGRUB_UINT64_T" bad_reads: %d\n",
+ cur, bad_reads);
+ grub_dprintf ("pmtimer", "timer is broken; giving up.\n");
+
+ if (bad_reads == 10)
+ return 0;
+ }
+#endif
+
+ if (cur < start)
+ cur += 0x100;
+
if (cur >= end)
{
- end_tsc = grub_get_tsc ();
+ grub_dprintf ("pmtimer", "pmtimer delta is 0x%"PRIxGRUB_UINT64_T"\n",
+ cur - start);
+ grub_dprintf ("pmtimer", "tsc delta is 0x%"PRIxGRUB_UINT64_T"\n",
+ end_tsc - start_tsc);
return end_
[PATCH v3 0/1] i386: Make pmtimer tsc calibration not take 51 seconds to fail
This is actually the fourth time this patch has been posted, but last time it was v2, so I guess we're zero-indexing. Changes from v2: - Rebase forward 6 months - Grammar pass over commit message to address Paul Menzel's review Be well, --Robbie Peter Jones (1): i386: Make pmtimer tsc calibration not take 51 seconds to fail grub-core/kern/i386/tsc_pmtimer.c | 112 -- 1 file changed, 92 insertions(+), 20 deletions(-) -- 2.35.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
