Page MenuHomeFreeBSD

busdma: deal with possible overflows of maxsize (+maths)
Needs ReviewPublic

Authored by bz on Jun 14 2022, 10:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 7:29 AM
Unknown Object (File)
Jan 11 2024, 5:52 PM
Unknown Object (File)
Dec 23 2023, 10:38 AM
Unknown Object (File)
Dec 15 2023, 4:35 AM
Unknown Object (File)
Dec 12 2023, 12:47 AM
Unknown Object (File)
Sep 22 2023, 10:53 PM
Unknown Object (File)
Apr 19 2023, 7:32 AM
Subscribers

Details

Summary

I am putting this up for the 2nd pair of eyes. When I was debugging
bounce problems on Arm64 a bit ago I noticed that there is a possible
overflow in bounce_bus_dmamap_create().

I've only later noticed similar code in X86 and am now wondering if this
is from an 32bit time still and c&p. I haven't looked.

The problem for various of these changes lies in the fact that
maxsize can be and is given as BUS_SPACE_MAXSIZE by drivers
and we are doing roundup2() on, often plus maths, which I wonder came
up because people didn't get results they wanted but again I don't know.

int pages;
pages = atop(roundup2(dmat->common.maxsize, PAGE_SIZE)) + 1;
pages = MIN(maxpages - bz->total_bpages, pages);

if maxsize is BUS_SPACE_MAXSIZE, pages will be 1 after the first line
and with the MIN in the next suddenly becomes a way lower number than
expected.

I believe in theory there is another problem there in that atop()
on a large enough maxsize may actually overflow int pages these days
{ (64 bit >> 12) still > 32bit }.
I simply changed the int to bus_size_t for sanity but it's probably
a while out and more a guardian to weird input.

In addition arm64 is trying to be extra careful to handle the
case of data spanning two pages even if possibly fitting in one
to my understanding and is doing that double in at least one case,
where maths also cause issues.

Whether all these changes as-are make sense -- I don't even think so
anymore now but at least someone else but me can have a look at these
bits now and see. x86 and rsicv were done by code inspection, I think
I never even tried to compile them. Other architectures I haven't
looked at.

Please add other people as you see fit.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45985
Build 42873: arc lint + arc unit

Event Timeline

bz requested review of this revision.Jun 14 2022, 10:54 PM

Needing more than 4GB of pages for a bounce zone seems like a different bug TBH and I'm not sure we want to permit quite that many pages. We might need to cap MAXSIZE at something more reasonable instead.

In D35490#808959, @jhb wrote:

Needing more than 4GB of pages for a bounce zone seems like a different bug TBH and I'm not sure we want to permit quite that many pages. We might need to cap MAXSIZE at something more reasonable instead.

Agreed for today's systems probably. Anyone who can will make sure they'll not bounce so lowering the MAX(maxsize) makes sense.

Probably both capping the input early but also making sure the follow-up maths are sane is needed so that we get reasonable number of pages.

I keep wondering if the slight variations are actually needed or could be "common-ified" as well and then that would all be a lot easier?