[SLOF] [PATCH 0/4] Block write support for SCSI and virtio-block disks

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 15.11.2016 03:54, Nikunj A Dadhania wrote:
>> Thomas Huth <thuth at redhat.com> writes:
>> 
>>> On 14.11.2016 08:53, Nikunj A Dadhania wrote:
>>>> Thomas Huth <thuth at redhat.com> writes:
>>>>
>>>>> On 14.11.2016 07:32, Nikunj A Dadhania wrote:
>>> [...]
>>>>>> My only worry here is that it would open up a way to write to the
>>>>>> critical section of the disk image from the SLOF prompt. Is there a way
>>>>>> we can prevent this?
>>>>>
>>>>> Good idea, I also felt a little bit uneasy to have write support in the
>>>>> firmware, but since GRUB needs it, we likely can't ignore this.
>>>>> So with critical section, you mean the MBR, I assume?
>>>>
>>>> Yes.
>>>>
>>>>> That should be feasible, I think I could add a check that refuses
>>>>> writes to the first 512 bytes (or a little bit more to also protect
>>>>> the GPT? Suggestions welcome!).
>>>>
>>>> Correct. For MBR 1st sector. For GPT (34 sectors in the beginning and 33
>>>> at the end) please refer to the following link for more details
>>>>
>>>> https://en.wikipedia.org/wiki/GUID_Partition_Table
>>>
>>> I just had a look at it, but adding code for checking whether the GPT is
>>> available or not (or using the checks from disk-label.fs) would render
>>> the whole checking mechanism quite complicated, as far as I can see...
>> 
>> : write 
>> \ IF sector number is 0 return
>> 
>> no-gpt? ! IF
>>     \ check-block number
>> THEN
>> 
>> \ call write
>> ;
>> 
>> Shouldn't something like the above work fine? Am I missing something?
>
> The problem is: The GPT stuff is handled at the disk-label.fs level, the
> sector write stuff is done at the driver / deblocker level instead.
> Let me try to paint an ASCII art picture:
>
>
>            +------------+
>            |            |
> ihandle ---> disk-label |
>            |            |
>            +------------+
>                  |
>       read/write |
>                  |
>            +-----v------+   read/write   +-----------+
>            |            +---------------->           |
>            |   driver   |                | deblocker |
>            | (SCSI...)  <----------------+           |
>            +------------+   read-blocks  +-----------+
>                  |          write-blocks
>                  v
>             lower layers
>
> The current offset is stored in the deblocker, the GPT information in
> the disk-label. Without adding some ugly hacks, there is currently now
> way to combine the offset information with the GPT information to do
> this check properly, as far as I can see.

Yeah, you are right.

>>> What about simply refusing write accesses to the first 4 sectors or so?
>>> Would that be OK?
>> 
>> That would not cover all GPT sectors
>
> OK. What about blocking writes at the driver level to the first 34
> sectors? According to the
> https://en.wikipedia.org/wiki/GUID_Partition_Table article, even with
> old MBR based layouts, the first partition normally never starts before
> LBA sector 63 (since it should be aligned on "cylinders"), so we should
> never end up writing to the first 34 sectors that could contain the
> GPT.

That makes sense and we can stop any writes to the first cylinder or
first 34 sectors

> Additionally I'll implement blocking of write access to the whole disk
> at the disk-label level, so you can only write if you opened the device
> tree node with a partition number specified. I think that should give us
> enough safety here.

Yes, that is a good check. Can we just use this? As our primary concern
is writing from SLOF prompt. So that will not allow me writing to first
63 cylinders.

Regards
Nikunj