Page MenuHomeFreeBSD

rtsock: Avoid copying uninitialized padding bytes
ClosedPublic

Authored by markj on Dec 22 2020, 7:04 PM.

Details

Reviewers
melifaro
ae
Group Reviewers
network
Summary

When copying sockaddrs out to userspace, we pad them to a multiple of
the platform alignment (sizeof(long)). However, some sockaddr sizes,
such as struct sockaddr_dl, are not an integer multiple of the
alignment, so we may end up copying out uninitialized bytes.

Fix this by always bouncing through a pre-zeroed sockaddr_storage.

Test Plan

This was found by KASAN.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 35626
Build 32521: arc lint + arc unit

Event Timeline

markj requested review of this revision.Dec 22 2020, 7:05 PM
markj added reviewers: network, melifaro, ae.

Well, I'd prefer to fix it by changing usable length of struct sockaddr_dl to be on 8-byte boundary, as it's actually dynamic.
Effectively the primary use case for it is storing the interface link-level address in the ifp ifaddrs list, so it can be a simple change in link_init_sdl().
The other part, gateway interfaces, already uses struct sockaddr_dl_short which is 16 bytes IIRC.

However, the change above requires more though/testing, so prob it's still good to have the safeguard described here.

If net/routing tests passes, then I'd say it's good to go.

This revision is now accepted and ready to land.Dec 22 2020, 8:00 PM

Well, I'd prefer to fix it by changing usable length of struct sockaddr_dl to be on 8-byte boundary, as it's actually dynamic.
Effectively the primary use case for it is storing the interface link-level address in the ifp ifaddrs list, so it can be a simple change in link_init_sdl().
The other part, gateway interfaces, already uses struct sockaddr_dl_short which is 16 bytes IIRC.

Wouldn't it also break the userspace ABI?

Well, I'd prefer to fix it by changing usable length of struct sockaddr_dl to be on 8-byte boundary, as it's actually dynamic.
Effectively the primary use case for it is storing the interface link-level address in the ifp ifaddrs list, so it can be a simple change in link_init_sdl().
The other part, gateway interfaces, already uses struct sockaddr_dl_short which is 16 bytes IIRC.

Wouldn't it also break the userspace ABI?

Unlikely. E.g. userspace applications will be able to access all necessary fields, size of the new structure will be less, so if they're copying it to an on-stack buffer, it will also work.
I don't currently see any obvious way of how it will break anything, but It's certainly worth testing some of the existing applications.

Well, I'd prefer to fix it by changing usable length of struct sockaddr_dl to be on 8-byte boundary, as it's actually dynamic.
Effectively the primary use case for it is storing the interface link-level address in the ifp ifaddrs list, so it can be a simple change in link_init_sdl().
The other part, gateway interfaces, already uses struct sockaddr_dl_short which is 16 bytes IIRC.

Wouldn't it also break the userspace ABI?

Unlikely. E.g. userspace applications will be able to access all necessary fields, size of the new structure will be less, so if they're copying it to an on-stack buffer, it will also work.

Ah, I thought you were proposing extending it.

I don't currently see any obvious way of how it will break anything, but It's certainly worth testing some of the existing applications.

There are some libc interfaces that use sizeof(struct sockaddr_dl), like getnameinfo(PF_LINK) and link_addr(), but it doesn't look like they'd be broken at least.