Skip to content
This repository was archived by the owner on Sep 18, 2020. It is now read-only.

loader/i386: fix out of bound memory copy on non-UEFI linux#53

Merged
ajeddeloh merged 1 commit intocoreos:2.02-coreosfrom
lucab:ups/suse-tpm
Apr 9, 2018
Merged

loader/i386: fix out of bound memory copy on non-UEFI linux#53
ajeddeloh merged 1 commit intocoreos:2.02-coreosfrom
lucab:ups/suse-tpm

Conversation

@lucab
Copy link
Copy Markdown
Contributor

@lucab lucab commented Apr 9, 2018

@lucab
Copy link
Copy Markdown
Contributor Author

lucab commented Apr 9, 2018

/cc @ajeddeloh

I noticed this in opensuse changelog while looking for an unrelated TPM thing. I'm not sure if it is related to your "free magic" investigation and I didn't test it, but it looks interesting.

Copy link
Copy Markdown
Contributor

@dm0- dm0- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly looks like the intended behavior. The upstream line this replaced was if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len).

len = sizeof (linux_params) - sizeof (lh);

grub_memcpy (&linux_params + sizeof (lh), kernel + kernel_offset, len);
grub_memcpy ((char *)&linux_params + sizeof (lh), kernel + kernel_offset, len);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being picky, the coding style puts a space after casts, but this is unlikely to be upstreamed since a newer implementation is being proposed, so not a big deal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I re-pushed to fix the spacing.

@ajeddeloh
Copy link
Copy Markdown
Contributor

Ran this through GDB and manually fixed the call to use the correct size. This fixes the free magic (can't continue booting since I'm running in 32-bit qemu for gdb's sake).

Copy link
Copy Markdown
Contributor

@ajeddeloh ajeddeloh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants