* [PATCH] fix detection of non-LUKS CRYPT
@ 2016-11-05 4:58 Corey Hickey
2016-11-05 12:31 ` Andrei Borzenkov
0 siblings, 1 reply; 7+ messages in thread
From: Corey Hickey @ 2016-11-05 4:58 UTC (permalink / raw)
To: grub-devel
Recent grub no longer detects non-LUKS ("plain dm-crypt") devices
properly. I think I tracked it down; the following patch fixes the
problem for me.
commit 4eb670dc50fe84012ec3e1f226ef9e94d8fa4b2b
Author: Corey Hickey <bugfood-c@fatooh.org>
Date: Sun Oct 30 22:35:32 2016 -0700
devmapper: fix detection of non-LUKS CRYPT
grub_util_get_dm_abstraction() does a string comparison of insufficient
length. When using a UUID such as "CRYPT-PLAIN-sda6_crypt", the function
returns GRUB_DEV_ABSTRACTION_LUKS.
This results in the error:
./grub-probe: error: disk `cryptouuid/sda6_crypt' not found.
This appears to be a copy/paste error introduced in:
a10e7a5a8918bea6e2632055129fa9b516fe965a
The bug was (apparently) latent until revealed by:
3bca85b4184f74995a7cc2791e432173fde26d34
Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 72e5582..a13a39c 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
grub_free (uuid);
return GRUB_DEV_ABSTRACTION_LVM;
}
- if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
+ if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
{
grub_free (uuid);
return GRUB_DEV_ABSTRACTION_LUKS;
Thanks,
Corey
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fix detection of non-LUKS CRYPT
2016-11-05 4:58 [PATCH] fix detection of non-LUKS CRYPT Corey Hickey
@ 2016-11-05 12:31 ` Andrei Borzenkov
2016-11-06 7:09 ` Corey Hickey
0 siblings, 1 reply; 7+ messages in thread
From: Andrei Borzenkov @ 2016-11-05 12:31 UTC (permalink / raw)
To: The development of GNU GRUB
05.11.2016 07:58, Corey Hickey пишет:
> Recent grub no longer detects non-LUKS ("plain dm-crypt") devices
> properly. I think I tracked it down; the following patch fixes the
> problem for me.
>
>
> commit 4eb670dc50fe84012ec3e1f226ef9e94d8fa4b2b
> Author: Corey Hickey <bugfood-c@fatooh.org>
> Date: Sun Oct 30 22:35:32 2016 -0700
>
> devmapper: fix detection of non-LUKS CRYPT
>
> grub_util_get_dm_abstraction() does a string comparison of insufficient
> length. When using a UUID such as "CRYPT-PLAIN-sda6_crypt", the function
> returns GRUB_DEV_ABSTRACTION_LUKS.
>
> This results in the error:
> ./grub-probe: error: disk `cryptouuid/sda6_crypt' not found.
>
> This appears to be a copy/paste error introduced in:
> a10e7a5a8918bea6e2632055129fa9b516fe965a
>
> The bug was (apparently) latent until revealed by:
> 3bca85b4184f74995a7cc2791e432173fde26d34
>
> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
>
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 72e5582..a13a39c 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
> grub_free (uuid);
> return GRUB_DEV_ABSTRACTION_LVM;
> }
> - if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
> + if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0
Committed, thanks! We really need some wrapper around (strncmp (foo,
"bar", sizeof ("bar") - 1), but for now it is OK as bug fix.
P.S. please in future either use git send-email or attach git
format-patch to make it easier to apply. Thank you.
> {
> grub_free (uuid);
> return GRUB_DEV_ABSTRACTION_LUKS;
>
>
> Thanks,
> Corey
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fix detection of non-LUKS CRYPT
2016-11-05 12:31 ` Andrei Borzenkov
@ 2016-11-06 7:09 ` Corey Hickey
2016-11-06 10:01 ` Andrei Borzenkov
0 siblings, 1 reply; 7+ messages in thread
From: Corey Hickey @ 2016-11-06 7:09 UTC (permalink / raw)
To: grub-devel
On 2016-11-05 05:31, Andrei Borzenkov wrote:
>> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
>> index 72e5582..a13a39c 100644
>> --- a/grub-core/osdep/devmapper/getroot.c
>> +++ b/grub-core/osdep/devmapper/getroot.c
>> @@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
>> grub_free (uuid);
>> return GRUB_DEV_ABSTRACTION_LVM;
>> }
>> - if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
>> + if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0
>
> Committed, thanks! We really need some wrapper around (strncmp (foo,
> "bar", sizeof ("bar") - 1), but for now it is OK as bug fix.
Excellent, you're welcome. That seemed like the most simple fix.
I took a stab at adding such a wrapper, but there are a ton of files
that could use it which I won't have a chance at being able to test. I
can send in an untested patch if you want...
> P.S. please in future either use git send-email or attach git
> format-patch to make it easier to apply. Thank you.
Got it.
Thanks,
Corey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fix detection of non-LUKS CRYPT
2016-11-06 7:09 ` Corey Hickey
@ 2016-11-06 10:01 ` Andrei Borzenkov
2016-11-07 6:48 ` Corey Hickey
0 siblings, 1 reply; 7+ messages in thread
From: Andrei Borzenkov @ 2016-11-06 10:01 UTC (permalink / raw)
To: grub-devel
06.11.2016 10:09, Corey Hickey пишет:
> On 2016-11-05 05:31, Andrei Borzenkov wrote:
>>> diff --git a/grub-core/osdep/devmapper/getroot.c
>>> b/grub-core/osdep/devmapper/getroot.c
>>> index 72e5582..a13a39c 100644
>>> --- a/grub-core/osdep/devmapper/getroot.c
>>> +++ b/grub-core/osdep/devmapper/getroot.c
>>> @@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
>>> grub_free (uuid);
>>> return GRUB_DEV_ABSTRACTION_LVM;
>>> }
>>> - if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
>>> + if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0
>>
>> Committed, thanks! We really need some wrapper around (strncmp (foo,
>> "bar", sizeof ("bar") - 1), but for now it is OK as bug fix.
>
> Excellent, you're welcome. That seemed like the most simple fix.
>
> I took a stab at adding such a wrapper, but there are a ton of files
> that could use it which I won't have a chance at being able to test. I
> can send in an untested patch if you want...
>
Large scale replacement will have to wait until release, but we sure can
discuss (and add) macro itself. I'm leaning towards simple
#define GRUB_IS_PREFIX(string,prefix) (strncmp((string), #prefix, sizeof
(#prefix) - 1) == 0)
Any other idea how to make it constant-safe?
>> P.S. please in future either use git send-email or attach git
>> format-patch to make it easier to apply. Thank you.
>
> Got it.
>
> Thanks,
> Corey
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fix detection of non-LUKS CRYPT
2016-11-06 10:01 ` Andrei Borzenkov
@ 2016-11-07 6:48 ` Corey Hickey
2016-11-07 11:04 ` Andrei Borzenkov
0 siblings, 1 reply; 7+ messages in thread
From: Corey Hickey @ 2016-11-07 6:48 UTC (permalink / raw)
To: The development of GNU GRUB
On 2016-11-06 02:01, Andrei Borzenkov wrote:
> 06.11.2016 10:09, Corey Hickey пишет:
>> On 2016-11-05 05:31, Andrei Borzenkov wrote:
>>>> diff --git a/grub-core/osdep/devmapper/getroot.c
>>>> b/grub-core/osdep/devmapper/getroot.c
>>>> index 72e5582..a13a39c 100644
>>>> --- a/grub-core/osdep/devmapper/getroot.c
>>>> +++ b/grub-core/osdep/devmapper/getroot.c
>>>> @@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
>>>> grub_free (uuid);
>>>> return GRUB_DEV_ABSTRACTION_LVM;
>>>> }
>>>> - if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
>>>> + if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0
>>>
>>> Committed, thanks! We really need some wrapper around (strncmp (foo,
>>> "bar", sizeof ("bar") - 1), but for now it is OK as bug fix.
>>
>> Excellent, you're welcome. That seemed like the most simple fix.
>>
>> I took a stab at adding such a wrapper, but there are a ton of files
>> that could use it which I won't have a chance at being able to test. I
>> can send in an untested patch if you want...
>>
>
> Large scale replacement will have to wait until release, but we sure can
> discuss (and add) macro itself. I'm leaning towards simple
>
> #define GRUB_IS_PREFIX(string,prefix) (strncmp((string), #prefix, sizeof
> (#prefix) - 1) == 0)
>
> Any other idea how to make it constant-safe?
I have to admit at this point that I never really got good at C, and
most of that was years ago. If I understand correctly, though, the
stringification in your example macro makes "foo" into "\"foo\"", before
sizeof() which would not be suitable.
My initial idea matched your earlier example:
#define STARTS_WITH(s1, s2) (strncmp((s1), (s2), sizeof(s2) - 1) == 0)
Am I failing to see a use case where that breaks?
The one bad thing I can see is that it can fail poorly if somebody
reverses the arguments by mistake. In that case, sizeof(s2) returns the
pointer size rather than the length of the string literal. I don't know
how to enforce that a macro argument be a string literal. An alternate
idea is:
#define STARTS_WITH(s1, s2) (strncmp((s1), (s2), strlen(s2)) == 0)
That should work with arguments in either order (though of course s2 is
expected to be smaller). I don't know of a drawback other than strlen
presumably being a bit slower, but I didn't get the impression any of
the code in question was performance-sensitive.
-Corey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fix detection of non-LUKS CRYPT
2016-11-07 6:48 ` Corey Hickey
@ 2016-11-07 11:04 ` Andrei Borzenkov
2016-11-12 6:29 ` Corey Hickey
0 siblings, 1 reply; 7+ messages in thread
From: Andrei Borzenkov @ 2016-11-07 11:04 UTC (permalink / raw)
To: The development of GNU GRUB
On Mon, Nov 7, 2016 at 9:48 AM, Corey Hickey <bugfood-ml@fatooh.org> wrote:
> On 2016-11-06 02:01, Andrei Borzenkov wrote:
>>
>> 06.11.2016 10:09, Corey Hickey пишет:
>>>
>>> On 2016-11-05 05:31, Andrei Borzenkov wrote:
>>>>>
>>>>> diff --git a/grub-core/osdep/devmapper/getroot.c
>>>>> b/grub-core/osdep/devmapper/getroot.c
>>>>> index 72e5582..a13a39c 100644
>>>>> --- a/grub-core/osdep/devmapper/getroot.c
>>>>> +++ b/grub-core/osdep/devmapper/getroot.c
>>>>> @@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
>>>>> grub_free (uuid);
>>>>> return GRUB_DEV_ABSTRACTION_LVM;
>>>>> }
>>>>> - if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
>>>>> + if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0
>>>>
>>>>
>>>> Committed, thanks! We really need some wrapper around (strncmp (foo,
>>>> "bar", sizeof ("bar") - 1), but for now it is OK as bug fix.
>>>
>>>
>>> Excellent, you're welcome. That seemed like the most simple fix.
>>>
>>> I took a stab at adding such a wrapper, but there are a ton of files
>>> that could use it which I won't have a chance at being able to test. I
>>> can send in an untested patch if you want...
>>>
>>
>> Large scale replacement will have to wait until release, but we sure can
>> discuss (and add) macro itself. I'm leaning towards simple
>>
>> #define GRUB_IS_PREFIX(string,prefix) (strncmp((string), #prefix, sizeof
>> (#prefix) - 1) == 0)
>>
>> Any other idea how to make it constant-safe?
>
>
> I have to admit at this point that I never really got good at C, and most of
> that was years ago. If I understand correctly, though, the stringification
> in your example macro makes "foo" into "\"foo\"", before sizeof() which
> would not be suitable.
>
> My initial idea matched your earlier example:
>
> #define STARTS_WITH(s1, s2) (strncmp((s1), (s2), sizeof(s2) - 1) == 0)
>
> Am I failing to see a use case where that breaks?
>
char *prefix = "CRYPT-LUKS1-";
sizeof (prefix) == 4 or 8 depending on platform. Not 13 in any case.
> The one bad thing I can see is that it can fail poorly if somebody reverses
> the arguments by mistake. In that case, sizeof(s2) returns the pointer size
> rather than the length of the string literal. I don't know how to enforce
> that a macro argument be a string literal. An alternate idea is:
>
> #define STARTS_WITH(s1, s2) (strncmp((s1), (s2), strlen(s2)) == 0)
>
This adds additional overhead of function call instead of constant
expression; unless we are sure gcc/clang are smart enough to reduce it
for constant arguments.
> That should work with arguments in either order (though of course s2 is
> expected to be smaller). I don't know of a drawback other than strlen
> presumably being a bit slower, but I didn't get the impression any of the
> code in question was performance-sensitive.
>
For user space it probably does not matter much; but for boot time
code it also increases code size and better be avoided.
Even for user space it is called often so if we can reduce overhead,
let's do it.
> -Corey
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fix detection of non-LUKS CRYPT
2016-11-07 11:04 ` Andrei Borzenkov
@ 2016-11-12 6:29 ` Corey Hickey
0 siblings, 0 replies; 7+ messages in thread
From: Corey Hickey @ 2016-11-12 6:29 UTC (permalink / raw)
To: The development of GNU GRUB
On 2016-11-07 03:04, Andrei Borzenkov wrote:
>> My initial idea matched your earlier example:
>>
>> #define STARTS_WITH(s1, s2) (strncmp((s1), (s2), sizeof(s2) - 1) == 0)
>>
>> Am I failing to see a use case where that breaks?
>>
>
> char *prefix = "CRYPT-LUKS1-";
> sizeof (prefix) == 4 or 8 depending on platform. Not 13 in any case.
Yes, the second argument would always have to be a string literal. Not
ideal, but I don't know a better way. Enforcing that the argument be a
string literal would work, but I can't find a way to do that. The best
I have come up with so far is this:
#define STARTS_WITH(s1, s2) (strncmp((s1), (s2 ""), sizeof(s2) - 1) == 0)
The string concatenation is intended to require s2 to be a string
literal and fail at compilation time otherwise. Unfortunately, it
is possible to defeat--s2 only has to _end_ with a string literal.
It breaks with something like this:
char *eight = "12345678"
char *nine = "123456789"
STARTS_WITH(eight, 1 ? nine : "foo"))
>> The one bad thing I can see is that it can fail poorly if somebody reverses
>> the arguments by mistake. In that case, sizeof(s2) returns the pointer size
>> rather than the length of the string literal. I don't know how to enforce
>> that a macro argument be a string literal. An alternate idea is:
>>
>> #define STARTS_WITH(s1, s2) (strncmp((s1), (s2), strlen(s2)) == 0)
>>
>
> This adds additional overhead of function call instead of constant
> expression; unless we are sure gcc/clang are smart enough to reduce it
> for constant arguments.
>
>> That should work with arguments in either order (though of course s2 is
>> expected to be smaller). I don't know of a drawback other than strlen
>> presumably being a bit slower, but I didn't get the impression any of the
>> code in question was performance-sensitive.
>>
>
> For user space it probably does not matter much; but for boot time
> code it also increases code size and better be avoided.
>
> Even for user space it is called often so if we can reduce overhead,
> let's do it.
I'm open to ideas.
-Corey
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-12 6:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-05 4:58 [PATCH] fix detection of non-LUKS CRYPT Corey Hickey
2016-11-05 12:31 ` Andrei Borzenkov
2016-11-06 7:09 ` Corey Hickey
2016-11-06 10:01 ` Andrei Borzenkov
2016-11-07 6:48 ` Corey Hickey
2016-11-07 11:04 ` Andrei Borzenkov
2016-11-12 6:29 ` Corey Hickey
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.