From 0b65bbfc878fe2a5c01c5d2cd08006b53fda7cf9 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Tue, 17 Jan 2023 23:21:39 +0400 Subject: [PATCH] fix: handle overwriting tags in syslinux ADV 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 --- .../bootloader/adv/syslinux/syslinux.go | 11 ++++++-- .../bootloader/adv/syslinux/syslinux_test.go | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux.go b/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux.go index d5ec89cdf..8947d8a1d 100644 --- a/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux.go +++ b/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux.go @@ -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 @@ -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 diff --git a/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux_test.go b/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux_test.go index d2616f4f0..4e3441c60 100644 --- a/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux_test.go +++ b/internal/app/machined/pkg/runtime/v1alpha1/bootloader/adv/syslinux/syslinux_test.go @@ -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") + } +}