On Tue, Mar 28, 2017 at 1:19 PM, Andrey Konovalov <andreyk...@google.com> wrote: > On Tue, Mar 28, 2017 at 5:54 PM, Craig Gallek <kraigatg...@gmail.com> wrote: >> On Tue, Mar 28, 2017 at 10:00 AM, Andrey Konovalov >> <andreyk...@google.com> wrote: >>> When calculating rb->frames_per_block * req->tp_block_nr the result >>> can overflow. >>> >>> Add a check that tp_block_size * tp_block_nr <= UINT_MAX. >>> >>> Since frames_per_block <= tp_block_size, the expression would >>> never overflow. >>> >>> Signed-off-by: Andrey Konovalov <andreyk...@google.com> >>> --- >>> net/packet/af_packet.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >>> index 506348abdf2f..c5c43fff8c01 100644 >>> --- a/net/packet/af_packet.c >>> +++ b/net/packet/af_packet.c >>> @@ -4197,6 +4197,9 @@ static int packet_set_ring(struct sock *sk, union >>> tpacket_req_u *req_u, >>> goto out; >>> if (unlikely(req->tp_frame_size == 0)) >>> goto out; >>> + if (unlikely((u64)req->tp_block_size * req->tp_block_nr > >>> + UINT_MAX)) >>> + goto out; >> So this may be pedantic, but really the only guarantee that you have >> for the 'unsigned int' type of these fields is that they are _at >> least_ 16 bits. There is no guarantee on the upper bound size, so >> casting to a u64 will be problematic on a compiler that happens to use >> 64 bits for 'unsigned int'. I'm not aware of any that use greater >> than 32 bits right now and using one that does may very well break >> other things in the kernel, but here we are... Perhaps a alternative >> fix would be to do the multiplication into an 'unsigned int' type and >> ensure that the result is larger than each of the original two values? > > I don't mind changing the check, but I've never encountered such compilers. > > Would this alternative work? It doesn't seem obvious. > > Other alternatives that I see for this check are: > > 1. req->tp_block_size > UINT_MAX / req->tp_block_nr > > 2. (req->tp_block_size * req->tp_block_nr) / req->tp_block_nr != > req->tp_block_size > > I'm not sure which one is better. I'm by no means the style expert here, but I would go with whichever makes the intention of the check (preventing overflow) most obvious. Maybe #1 in your example? I'm also not sure what the acceptable assumptions about the size of 'int' are in the kernel code. I'm sure there's a thread out there with Linus expressing a strong feeling one way or another, but I haven't found it yet ;)
> >> >> The real issue is that explicit size types should have been used in >> this userspace structure.
