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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.12 at 18:43, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> On Fri, 2012-08-03 at 16:22 +0100, Jan Beulich wrote:
>> >>> On 03.08.12 at 16:50, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel