* [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector @ 2012-08-03 14:50 Frediano Ziglio 2012-08-03 15:09 ` David Vrabel 2012-08-03 15:22 ` Jan Beulich 0 siblings, 2 replies; 6+ messages in thread From: Frediano Ziglio @ 2012-08-03 14:50 UTC (permalink / raw) To: xen-devel@lists.xen.org If a 4k disk is used for first BIOS disk loader corrupt itself. This patch increase sector buffer in order to avoid this overflow Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com> --- xen/arch/x86/boot/edd.S | 2 +- xen/arch/x86/boot/trampoline.S | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S index 2c8df8c..1c802a6 100644 --- a/xen/arch/x86/boot/edd.S +++ b/xen/arch/x86/boot/edd.S @@ -154,4 +154,4 @@ boot_mbr_signature_nr: boot_mbr_signature: .fill EDD_MBR_SIG_MAX*8,1,0 boot_edd_info: - .fill 512,1,0 # big enough for a disc sector + .fill 4096,1,0 # big enough for a disc sector diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index 4421fc2..bd54c9e 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -224,6 +224,6 @@ skip_realmode: rm_idt: .word 256*4-1, 0, 0 #include "mem.S" -#include "edd.S" #include "video.S" #include "wakeup.S" +#include "edd.S" -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector 2012-08-03 14:50 [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector Frediano Ziglio @ 2012-08-03 15:09 ` David Vrabel 2012-08-03 15:22 ` Jan Beulich 1 sibling, 0 replies; 6+ messages in thread From: David Vrabel @ 2012-08-03 15:09 UTC (permalink / raw) To: Frediano Ziglio; +Cc: xen-devel@lists.xen.org On 03/08/12 15:50, Frediano Ziglio wrote: > > If a 4k disk is used for first BIOS disk loader corrupt itself. > > This patch increase sector buffer in order to avoid this overflow > > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com> > --- > xen/arch/x86/boot/edd.S | 2 +- > xen/arch/x86/boot/trampoline.S | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S > index 2c8df8c..1c802a6 100644 > --- a/xen/arch/x86/boot/edd.S > +++ b/xen/arch/x86/boot/edd.S > @@ -154,4 +154,4 @@ boot_mbr_signature_nr: > boot_mbr_signature: > .fill EDD_MBR_SIG_MAX*8,1,0 > boot_edd_info: > - .fill 512,1,0 # big enough for a disc sector > + .fill 4096,1,0 # big enough for a disc sector Can we get a #define for this value? > diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S > index 4421fc2..bd54c9e 100644 > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -224,6 +224,6 @@ skip_realmode: > rm_idt: .word 256*4-1, 0, 0 > > #include "mem.S" > -#include "edd.S" > #include "video.S" > #include "wakeup.S" > +#include "edd.S" This part looks unnecessary. Included by mistake? David ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector 2012-08-03 14:50 [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector Frediano Ziglio 2012-08-03 15:09 ` David Vrabel @ 2012-08-03 15:22 ` Jan Beulich 2012-08-03 16:43 ` Frediano Ziglio 2012-08-07 12:08 ` Jan Beulich 1 sibling, 2 replies; 6+ messages in thread From: Jan Beulich @ 2012-08-03 15:22 UTC (permalink / raw) To: Frediano Ziglio; +Cc: xen-devel@lists.xen.org >>> On 03.08.12 at 16:50, Frediano Ziglio <frediano.ziglio@citrix.com> wrote: > If a 4k disk is used for first BIOS disk loader corrupt itself. If such is really permitted by the specification (which I doubt it is for the standard, old-style INT13 functions - it's a different story for functions 42 and 43, where the caller can be expected to call function 48 first). > This patch increase sector buffer in order to avoid this overflow And if we indeed need to adjust for this, then let's fix this properly: Don't just increase the buffer size, but also check that the sector size reported actually fits. That may require calling Fn48 first, before doing the actual read. > --- a/xen/arch/x86/boot/edd.S > +++ b/xen/arch/x86/boot/edd.S > @@ -154,4 +154,4 @@ boot_mbr_signature_nr: > boot_mbr_signature: > .fill EDD_MBR_SIG_MAX*8,1,0 > boot_edd_info: > - .fill 512,1,0 # big enough for a disc sector > + .fill 4096,1,0 # big enough for a disc sector Also I wonder whether it wouldn't be more smart to re-use the wakeup stack (which is already 4k in size), and shrink this buffer to the maximum size ever used without reading sectors into it (EDD_INFO_MAX*(EDDEXTSIZE+EDDPARMSIZE)). > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -224,6 +224,6 @@ skip_realmode: > rm_idt: .word 256*4-1, 0, 0 > > #include "mem.S" > -#include "edd.S" > #include "video.S" > #include "wakeup.S" > +#include "edd.S" Finally, you should also explain why this change is needed. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector 2012-08-03 15:22 ` Jan Beulich @ 2012-08-03 16:43 ` Frediano Ziglio 2012-08-06 7:54 ` Jan Beulich 2012-08-07 12:08 ` Jan Beulich 1 sibling, 1 reply; 6+ messages in thread From: Frediano Ziglio @ 2012-08-03 16:43 UTC (permalink / raw) To: JBeulich@suse.com; +Cc: xen-devel@lists.xen.org On Fri, 2012-08-03 at 16:22 +0100, Jan Beulich wrote: > >>> On 03.08.12 at 16:50, Frediano Ziglio <frediano.ziglio@citrix.com> wrote: > > If a 4k disk is used for first BIOS disk loader corrupt itself. > > If such is really permitted by the specification (which I doubt it > is for the standard, old-style INT13 functions - it's a different > story for functions 42 and 43, where the caller can be expected > to call function 48 first). > I don't know. They always speaks about sector size and in int13/5 for floppy you can specify different sector sizes (up to 1024). > > This patch increase sector buffer in order to avoid this overflow > > And if we indeed need to adjust for this, then let's fix this properly: > Don't just increase the buffer size, but also check that the sector > size reported actually fits. That may require calling Fn48 first, > before doing the actual read. > Or read to a location of memory we are sure there is enough space (something like 2000:0000). > > --- a/xen/arch/x86/boot/edd.S > > +++ b/xen/arch/x86/boot/edd.S > > @@ -154,4 +154,4 @@ boot_mbr_signature_nr: > > boot_mbr_signature: > > .fill EDD_MBR_SIG_MAX*8,1,0 > > boot_edd_info: > > - .fill 512,1,0 # big enough for a disc sector > > + .fill 4096,1,0 # big enough for a disc sector > > Also I wonder whether it wouldn't be more smart to re-use the > wakeup stack (which is already 4k in size), and shrink this buffer > to the maximum size ever used without reading sectors into it > (EDD_INFO_MAX*(EDDEXTSIZE+EDDPARMSIZE)). > Yes, reusing this buffer could be useful. It could be also useful to put it at the end of the trampoline code in order to try avoiding future problems if sector size grow. > > --- a/xen/arch/x86/boot/trampoline.S > > +++ b/xen/arch/x86/boot/trampoline.S > > @@ -224,6 +224,6 @@ skip_realmode: > > rm_idt: .word 256*4-1, 0, 0 > > > > #include "mem.S" > > -#include "edd.S" > > #include "video.S" > > #include "wakeup.S" > > +#include "edd.S" > > Finally, you should also explain why this change is needed. > This is to move the buffer at the end and avoid overflowing to other code. > Jan > Frediano ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector 2012-08-03 16:43 ` Frediano Ziglio @ 2012-08-06 7:54 ` Jan Beulich 0 siblings, 0 replies; 6+ messages in thread From: Jan Beulich @ 2012-08-06 7:54 UTC (permalink / raw) To: Frediano Ziglio; +Cc: xen-devel@lists.xen.org >>> On 03.08.12 at 18:43, Frediano Ziglio <frediano.ziglio@citrix.com> wrote: > On Fri, 2012-08-03 at 16:22 +0100, Jan Beulich wrote: >> >>> On 03.08.12 at 16:50, Frediano Ziglio <frediano.ziglio@citrix.com> wrote: >> > If a 4k disk is used for first BIOS disk loader corrupt itself. >> >> If such is really permitted by the specification (which I doubt it >> is for the standard, old-style INT13 functions - it's a different >> story for functions 42 and 43, where the caller can be expected >> to call function 48 first). >> > > I don't know. They always speaks about sector size and in int13/5 for > floppy you can specify different sector sizes (up to 1024). This is the format operation (and you can't do the same for read/write without adjusting some memory variables). Plus, as you say, this is a floppy specific thing. I'm unaware of the old INT13 interface allowing other than 512-byte sectors. Did you check with the vendor of the machine/BIOS? >> > This patch increase sector buffer in order to avoid this overflow >> >> And if we indeed need to adjust for this, then let's fix this properly: >> Don't just increase the buffer size, but also check that the sector >> size reported actually fits. That may require calling Fn48 first, >> before doing the actual read. >> > > Or read to a location of memory we are sure there is enough space > (something like 2000:0000). No, please let's not start using fixed addresses again. If anything, you need to consult the memory map to see what area of memory is safe to use. >> > --- a/xen/arch/x86/boot/edd.S >> > +++ b/xen/arch/x86/boot/edd.S >> > @@ -154,4 +154,4 @@ boot_mbr_signature_nr: >> > boot_mbr_signature: >> > .fill EDD_MBR_SIG_MAX*8,1,0 >> > boot_edd_info: >> > - .fill 512,1,0 # big enough for a disc > sector >> > + .fill 4096,1,0 # big enough for a disc > sector >> >> Also I wonder whether it wouldn't be more smart to re-use the >> wakeup stack (which is already 4k in size), and shrink this buffer >> to the maximum size ever used without reading sectors into it >> (EDD_INFO_MAX*(EDDEXTSIZE+EDDPARMSIZE)). >> > > Yes, reusing this buffer could be useful. It could be also useful to put > it at the end of the trampoline code in order to try avoiding future > problems if sector size grow. Putting it at the end doesn't help in any way - you'd then risk corrupting the EBDA or other BIOS/firmware data. >> > --- a/xen/arch/x86/boot/trampoline.S >> > +++ b/xen/arch/x86/boot/trampoline.S >> > @@ -224,6 +224,6 @@ skip_realmode: >> > rm_idt: .word 256*4-1, 0, 0 >> > >> > #include "mem.S" >> > -#include "edd.S" >> > #include "video.S" >> > #include "wakeup.S" >> > +#include "edd.S" >> >> Finally, you should also explain why this change is needed. >> > > This is to move the buffer at the end and avoid overflowing to other > code. As said - this should be clarified in the change set comment and doesn't really help. The only thing helping being safe going forward is - determine the sector size - either don't do I/O when it's too large, or dynamically determine a safe buffer location. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector 2012-08-03 15:22 ` Jan Beulich 2012-08-03 16:43 ` Frediano Ziglio @ 2012-08-07 12:08 ` Jan Beulich 1 sibling, 0 replies; 6+ messages in thread From: Jan Beulich @ 2012-08-07 12:08 UTC (permalink / raw) To: Frediano Ziglio; +Cc: xen-devel@lists.xen.org >>> On 03.08.12 at 17:22, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> On 03.08.12 at 16:50, Frediano Ziglio <frediano.ziglio@citrix.com> wrote: >> This patch increase sector buffer in order to avoid this overflow > > And if we indeed need to adjust for this, then let's fix this properly: > Don't just increase the buffer size, but also check that the sector > size reported actually fits. That may require calling Fn48 first, > before doing the actual read. This, btw, is how current Linux is doing it. But I don't think they're really protected from corruption either when sufficiently large sector sizes get encountered... Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-07 12:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-03 14:50 [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector Frediano Ziglio 2012-08-03 15:09 ` David Vrabel 2012-08-03 15:22 ` Jan Beulich 2012-08-03 16:43 ` Frediano Ziglio 2012-08-06 7:54 ` Jan Beulich 2012-08-07 12:08 ` Jan Beulich
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.