If guest kernel or ramdisk image is too large to fit in the dom0 memory, it
causes pygrub crash with "out of memory" error.
This patch reads kernel/ramdisk file in 1 MB blocks so prevent exhausting whole
memory.
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
Patch:
--
diff -r 238900a4ed22 tools/pygrub/src/pygrub
--- a/tools/pygrub/src/pygrub Mon May 21 12:03:32 2012 +0200
+++ b/tools/pygrub/src/pygrub Tue May 22 11:32:28 2012 +0200
@@ -697,6 +697,20 @@
def usage():
print >> sys.stderr, "Usage: %s [-q|--quiet]
[-i|--interactive] [-n|--not-really] [--output=] [--kernel=] [--ramdisk=]
[--args=] [--entry=] [--output-directory=] [--output-format=sxp|simple|simple0]
<image>" %(sys.argv[0],)
+ def copy_from_image(fs, file_to_copy, tmp_prefix):
+ (tfd, rv) = tempfile.mkstemp(prefix=tmp_prefix,
dir="/var/lib/xen")
+ file_handle = fs.open_file(file_to_copy)
+ readsize = 0
+
+ while True:
+ data = file_handle.read(1048576,readsize)
+ if len(data) == 0:
+ os.close(tfd)
+ return rv
+
+ readsize += len(data)
+ os.write(tfd,data)
+
try:
opts, args = getopt.gnu_getopt(sys.argv[1:],
''qinh::'',
["quiet", "interactive",
"not-really", "help",
@@ -824,21 +838,15 @@
if not_really:
bootcfg["kernel"] = "<kernel:%s>" %
chosencfg["kernel"]
else:
- data = fs.open_file(chosencfg["kernel"]).read()
- (tfd, bootcfg["kernel"]) =
tempfile.mkstemp(prefix="boot_kernel.",
- dir=output_directory)
- os.write(tfd, data)
- os.close(tfd)
+ bootcfg["kernel"] =
copy_from_image(fs,chosencfg["kernel"],
+ "boot_kernel.")
if chosencfg["ramdisk"]:
if not_really:
bootcfg["ramdisk"] = "<ramdisk:%s>" %
chosencfg["ramdisk"]
else:
- data = fs.open_file(chosencfg["ramdisk"],).read()
- (tfd, bootcfg["ramdisk"]) = tempfile.mkstemp(
- prefix="boot_ramdisk.", dir=output_directory)
- os.write(tfd, data)
- os.close(tfd)
+ bootcfg["ramdisk"] = copy_from_image(fs,
chosencfg["ramdisk",
+ "boot_ramdisk.")
else:
initrd = None
On Tue, 2012-05-22 at 08:37 +0100, Miroslav Rezanina wrote:> If guest kernel or ramdisk image is too large to fit in the dom0 memory, it causes pygrub crash with "out of memory" error. > This patch reads kernel/ramdisk file in 1 MB blocks so prevent exhausting whole memory.Thanks, we''ve had a similar patch from Michael Young (CCd) too, see: <alpine.DEB.2.00.1205170029550.26049@vega-c.dur.ac.uk>. They look functionally pretty similar, one big difference is that Michael limits the size of the cfg file as well, which seems wise. Looks like his handles errors on the os.write too? I do like splitting the read loop out into a function as you''ve done though.> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > Patch: > -- > diff -r 238900a4ed22 tools/pygrub/src/pygrub > --- a/tools/pygrub/src/pygrub Mon May 21 12:03:32 2012 +0200 > +++ b/tools/pygrub/src/pygrub Tue May 22 11:32:28 2012 +0200 > @@ -697,6 +697,20 @@ > def usage(): > print >> sys.stderr, "Usage: %s [-q|--quiet] [-i|--interactive] [-n|--not-really] [--output=] [--kernel=] [--ramdisk=] [--args=] [--entry=] [--output-directory=] [--output-format=sxp|simple|simple0] <image>" %(sys.argv[0],) > > + def copy_from_image(fs, file_to_copy, tmp_prefix): > + (tfd, rv) = tempfile.mkstemp(prefix=tmp_prefix, dir="/var/lib/xen") > + file_handle = fs.open_file(file_to_copy) > + readsize = 0 > + > + while True: > + data = file_handle.read(1048576,readsize) > + if len(data) == 0: > + os.close(tfd) > + return rv > + > + readsize += len(data) > + os.write(tfd,data) > + > try: > opts, args = getopt.gnu_getopt(sys.argv[1:], ''qinh::'', > ["quiet", "interactive", "not-really", "help", > @@ -824,21 +838,15 @@ > if not_really: > bootcfg["kernel"] = "<kernel:%s>" % chosencfg["kernel"] > else: > - data = fs.open_file(chosencfg["kernel"]).read() > - (tfd, bootcfg["kernel"]) = tempfile.mkstemp(prefix="boot_kernel.", > - dir=output_directory) > - os.write(tfd, data) > - os.close(tfd) > + bootcfg["kernel"] = copy_from_image(fs,chosencfg["kernel"], > + "boot_kernel.") > > if chosencfg["ramdisk"]: > if not_really: > bootcfg["ramdisk"] = "<ramdisk:%s>" % chosencfg["ramdisk"] > else: > - data = fs.open_file(chosencfg["ramdisk"],).read() > - (tfd, bootcfg["ramdisk"]) = tempfile.mkstemp( > - prefix="boot_ramdisk.", dir=output_directory) > - os.write(tfd, data) > - os.close(tfd) > + bootcfg["ramdisk"] = copy_from_image(fs, chosencfg["ramdisk", > + "boot_ramdisk.") > else: > initrd = None > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Tue, 22 May 2012, Ian Campbell wrote:> On Tue, 2012-05-22 at 08:37 +0100, Miroslav Rezanina wrote: >> If guest kernel or ramdisk image is too large to fit in the dom0 memory, it causes pygrub crash with "out of memory" error. >> This patch reads kernel/ramdisk file in 1 MB blocks so prevent exhausting whole memory. > > Thanks, we''ve had a similar patch from Michael Young (CCd) too, see: > <alpine.DEB.2.00.1205170029550.26049@vega-c.dur.ac.uk>.or at http://lists.xen.org/archives/html/xen-devel/2012-05/msg01183.html> They look functionally pretty similar, one big difference is that > Michael limits the size of the cfg file as well, which seems wise.Yes, a malicious guest could have a huge grub configuration file as well. My strategy was just to read the first megabyte as I can''t see why a legitimate configuration file would be anywhere near that long.> Looks like his handles errors on the os.write too?Yes, if there are write problems my patch deletes the kernel and ramdisk temporary files and exits with an error. This isn''t so important though as the calling process should clean them up afterwards though my testing suggested that if the write failed due to lack of space, the guest may try to run with incomplete kernel or ramdisk files, and also you would have a full filesystem for a bit longer.> I do like splitting the read loop out into a function as you''ve done > though.Yes, that does seem neater. Michael Young
----- Original Message -----> From: "M A Young" <m.a.young@durham.ac.uk> > To: "Ian Campbell" <Ian.Campbell@citrix.com> > Cc: "Miroslav Rezanina" <mrezanin@redhat.com>, "xen-devel" <xen-devel@lists.xensource.com> > Sent: Tuesday, May 22, 2012 12:30:31 PM > Subject: Re: [Xen-devel] [PATCH] Do not read files at once in pygrub > > On Tue, 22 May 2012, Ian Campbell wrote: > > > On Tue, 2012-05-22 at 08:37 +0100, Miroslav Rezanina wrote: > >> If guest kernel or ramdisk image is too large to fit in the dom0 > >> memory, it causes pygrub crash with "out of memory" error. > >> This patch reads kernel/ramdisk file in 1 MB blocks so prevent > >> exhausting whole memory. > > > > Thanks, we''ve had a similar patch from Michael Young (CCd) too, > > see: > > <alpine.DEB.2.00.1205170029550.26049@vega-c.dur.ac.uk>. > or at > http://lists.xen.org/archives/html/xen-devel/2012-05/msg01183.html > > > They look functionally pretty similar, one big difference is that > > Michael limits the size of the cfg file as well, which seems wise. > > Yes, a malicious guest could have a huge grub configuration file as > well. > My strategy was just to read the first megabyte as I can''t see why a > legitimate configuration file would be anywhere near that long. >Yeah, it should not be as big. However, I think it should use same approach as kernel/ramdisk in case there will be such a big configuration file (even we do not see reason now).> > Looks like his handles errors on the os.write too? > > Yes, if there are write problems my patch deletes the kernel and > ramdisk > temporary files and exits with an error. This isn''t so important > though as > the calling process should clean them up afterwards though my testing > suggested that if the write failed due to lack of space, the guest > may try > to run with incomplete kernel or ramdisk files, and also you would > have a > full filesystem for a bit longer. >This is useful part as I sometimes experienced leftovers in case of errors.> > I do like splitting the read loop out into a function as you''ve > > done > > though. > > Yes, that does seem neater. > > Michael Young >
On Tue, 22 May 2012, Miroslav Rezanina wrote:> ----- Original Message ----- >> From: "M A Young" <m.a.young@durham.ac.uk> >> To: "Ian Campbell" <Ian.Campbell@citrix.com> >> Cc: "Miroslav Rezanina" <mrezanin@redhat.com>, "xen-devel" <xen-devel@lists.xensource.com> >> Sent: Tuesday, May 22, 2012 12:30:31 PM >> Subject: Re: [Xen-devel] [PATCH] Do not read files at once in pygrub >> >> On Tue, 22 May 2012, Ian Campbell wrote: >>> They look functionally pretty similar, one big difference is that >>> Michael limits the size of the cfg file as well, which seems wise. >> >> Yes, a malicious guest could have a huge grub configuration file as >> well. >> My strategy was just to read the first megabyte as I can''t see why a >> legitimate configuration file would be anywhere near that long. >> > Yeah, it should not be as big. However, I think it should use same approach > as kernel/ramdisk in case there will be such a big configuration file (even > we do not see reason now).They are already different as the configuration file is only read into memory and processed there, whereas the kernel and ramdisk are just file copies ready for the calling process to use. I haven''t specifically looked at this bit of the code but I think it could be difficult to protect against a malicious configuration file without truncating it somewhere as for example, you have to consider the case of a legitimately formatted grub file with, say, a million menu entries. Michael Young