[PATCH] Warn the user to edit environment block by grub-editenv

[PATCH] Warn the user to edit environment block by grub-editenv

[Michael Chang]

The environment block is a preallocated 1024-byte file serves as persistent
storage for environment variables. It has its own format which is sensitive to
corruption if using editor doesn't know how to process it. Besides the editor
may inadvertantly change size allocation that could have it sparse the
filesystem which could lead to unexpected outcome.

This patch adds message in grubenv file to warn the user from editing it by
tools other than grub-editenv.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/editenv.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/util/editenv.c b/util/editenv.c
index eb2d0c03a..ee4c9e53f 100644
--- a/util/editenv.c
+++ b/util/editenv.c
@@ -30,12 +30,14 @@
 #include <string.h>
 
 #define DEFAULT_ENVBLK_SIZE	1024
+#define GRUB_ENVBLK_MESSAGE	"# WARNING: Do not edit this file other than by "PACKAGE"-editenv\n"
 
 void
 grub_util_create_envblk_file (const char *name)
 {
   FILE *fp;
   char *buf;
+  char *pbuf;
   char *namenew;
 
   buf = xmalloc (DEFAULT_ENVBLK_SIZE);
@@ -46,9 +48,13 @@ grub_util_create_envblk_file (const char *name)
     grub_util_error (_("cannot open `%s': %s"), namenew,
 		     strerror (errno));
 
-  memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
-  memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#',
-          DEFAULT_ENVBLK_SIZE - sizeof (GRUB_ENVBLK_SIGNATURE) + 1);
+  pbuf = buf;
+  memcpy (pbuf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
+  pbuf += sizeof (GRUB_ENVBLK_SIGNATURE) - 1;
+  memcpy (pbuf, GRUB_ENVBLK_MESSAGE, sizeof (GRUB_ENVBLK_MESSAGE) - 1);
+  pbuf += sizeof (GRUB_ENVBLK_MESSAGE) - 1;
+  memset (pbuf , '#',
+          DEFAULT_ENVBLK_SIZE - sizeof (GRUB_ENVBLK_SIGNATURE) - sizeof (GRUB_ENVBLK_MESSAGE) + 2);
 
   if (fwrite (buf, 1, DEFAULT_ENVBLK_SIZE, fp) != DEFAULT_ENVBLK_SIZE)
     grub_util_error (_("cannot write to `%s': %s"), namenew,
-- 
2.16.4

Re: [PATCH] Warn the user to edit environment block by grub-editenv

[Javier Martinez Canillas]

Hello Michael,

On 11/5/19 10:20 AM, Michael Chang wrote:
> The environment block is a preallocated 1024-byte file serves as persistent
> storage for environment variables. It has its own format which is sensitive to
> corruption if using editor doesn't know how to process it. Besides the editor
> may inadvertantly change size allocation that could have it sparse the
> filesystem which could lead to unexpected outcome.
>

Yes, we also get reports about this issue: https://bugzilla.redhat.com/show_bug.cgi?id=1625124
 
> This patch adds message in grubenv file to warn the user from editing it by
> tools other than grub-editenv.
> 

Agreed that users should be warned that the file should only be edited with
the GRUB tools.

I also wonder if grub-editenv can't be made more robust and correct a grubenv
file (i.e: filling the missing # characters / removing any extra # character)
if is found to be corrupted and not having the expected size.

> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  util/editenv.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

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

Re: [PATCH] Warn the user to edit environment block by grub-editenv

[Michael Chang]

On Tue, Nov 05, 2019 at 12:49:08PM +0100, Javier Martinez Canillas wrote:
> Hello Michael,
> 
> On 11/5/19 10:20 AM, Michael Chang wrote:
> > The environment block is a preallocated 1024-byte file serves as persistent
> > storage for environment variables. It has its own format which is sensitive to
> > corruption if using editor doesn't know how to process it. Besides the editor
> > may inadvertantly change size allocation that could have it sparse the
> > filesystem which could lead to unexpected outcome.
> >
> 
> Yes, we also get reports about this issue: https://bugzilla.redhat.com/show_bug.cgi?id=1625124
>  
> > This patch adds message in grubenv file to warn the user from editing it by
> > tools other than grub-editenv.
> > 
> 
> Agreed that users should be warned that the file should only be edited with
> the GRUB tools.
> 
> I also wonder if grub-editenv can't be made more robust and correct a grubenv
> file (i.e: filling the missing # characters / removing any extra # character)
> if is found to be corrupted and not having the expected size.

IMHO when people trying to read grubenv file they didn't expect the
sanitization from happening, as that might be troublesome to describe
problem if anyone wants to troubleshoot issue related to malformed
grubenv which is just victim of filesystem corruption and like.

I personally wouldn't mind if the function is provided as separate
command and backup original file somewhere ..

> 
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > ---
> >  util/editenv.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> 
> The patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks a lot for your feedback and review. :)

Regards,
Michael

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

Re: [PATCH] Warn the user to edit environment block by grub-editenv

[Daniel Kiper]

On Tue, Nov 05, 2019 at 09:20:22AM +0000, Michael Chang wrote:
> The environment block is a preallocated 1024-byte file serves as persistent
> storage for environment variables. It has its own format which is sensitive to
> corruption if using editor doesn't know how to process it. Besides the editor
> may inadvertantly change size allocation that could have it sparse the
> filesystem which could lead to unexpected outcome.
>
> This patch adds message in grubenv file to warn the user from editing it by
> tools other than grub-editenv.
>
> Signed-off-by: Michael Chang <mchang@suse.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel