Skip to content

Commit

Permalink
fix: handle overwriting tags in syslinux ADV
Browse files Browse the repository at this point in the history
This is (still) being used in Talos to handle upgrade rollbacks.

There were multiple problems with this code, and one of them leads to
panic if the tag is written multiple times without deletion:

```
github.com/siderolabs/talos/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux.ADV.SetTagBytes({0xc00175bc00?, 0x1f11dbe?, 0xed4f4d?}, 0x0?, {0xc000afb7f0?, 0x400?, 0x0?})
/src/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux.go:125 +0x270
github.com/siderolabs/talos/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux.ADV.SetTag(...)
/src/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux.go:95
github.com/siderolabs/talos/cmd/installer/pkg/install.(*Installer).Install(0xc0004374a0, 0x5)
/src/cmd/installer/pkg/install/install.go
```

The `uint8()` conversion was causing overflow and wrong index when ADV
real length is over 255.

Fix multiple writes of the same tag by deleting previous value first.

Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
  • Loading branch information
smira committed Jan 17, 2023
1 parent 70d9428 commit 0b65bbf
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ func (a ADV) SetTagBytes(t uint8, val []byte) (ok bool) {
return false
}

// delete the tag if it exists
a.DeleteTag(t)

// Header is in first 8 bytes.
i := 8

Expand All @@ -116,13 +119,17 @@ func (a ADV) SetTagBytes(t uint8, val []byte) (ok bool) {
continue
}

// overflow check
if i+2+len(val) > AdvSize-4-2 {
return false
}

length := uint8(len(val))

a[i] = t

a[i+1] = length

copy(a[i+2:uint8(i+2)+length], val)
copy(a[i+2:i+2+int(length)], val)

ok = true

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,3 +324,31 @@ func TestADV_tail(t *testing.T) {
})
}
}

func TestADV_overwrite(t *testing.T) {
buf := make([]byte, 2*AdvSize)

a, err := NewADV(bytes.NewReader(buf))
if err != nil {
t.Errorf("NewADV() failed: %s", err)
}

for i := 0; i < 1024; i++ {
if !a.SetTag(adv.Bootonce, "yes") {
t.Errorf("SetTag() failed")
}
}
}

func TestADV_many_tags(t *testing.T) {
buf := make([]byte, 2*AdvSize)

a, err := NewADV(bytes.NewReader(buf))
if err != nil {
t.Errorf("NewADV() failed: %s", err)
}

for i := uint8(1); i < 255; i++ {
a.SetTag(i, "xa")
}
}

0 comments on commit 0b65bbf

Please sign in to comment.