Xen devel - Aug 2012 - [PATCH] Increment buffer used to read first boot sector in order to accomodate space for 4k sector

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

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

>>> 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

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

>>> 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

>>> 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