Consider specify lower limit for conv2d/pool2d kernel sizes, dilations, strides

[philloooo]

During our testing with chromium implementation, we found more edge cases in complex ops like pool2d and conv2d, where when specifying some unreasonably large sizes of these parameters trigger integer overflows in some of the backends.

In reality, these parameters never have too large values.
If we limit them to smaller and realistic upper bounds that are reasonable for all use cases, it can reduce our attack surface and also make our fuzz testing much more efficient.

WDYT?

We should also go over other complex ops to see where we can harden the limits.

Initial thoughts on size limit:
kernel size: 1024
strides & dilations: 256

@fdwr @huningxin ?

[huningxin]

Initial thoughts on size limit:
kernel size: 1024
strides & dilations: 256

Thanks @philloooo , looks reasonable. I'll check internally and get back. Should we also have padding size limit?

[fdwr]

specifying some unreasonably large sizes of these parameters trigger integer overflows

Phillis: What backend behavior do you get in such cases?

• API Error?
• Illegal memory access (access violation and process crash)?
• Incorrect output tensor values?

Rather than limit these attributes independently, is it not more useful to limit the calculated total breadth of the kernel across the input tensor? (the "effective kernel size") See, given a 1D 16'384 element tensor, a caller should legally be to apply a 2-wide kernel filter with dilation of 512 elements (meaning a total span of 513 input elements for each step convolved), but that would be invalid with these proposed new limits. However, a 512-wide kernel with dilations of 256 would stretch across 131'073 input elements, which wouldn't make any sense given there are only 16'384 elements, and yet those values would be valid per the proposed limits.

Would the cases you've seen (curious to see some examples encountered) be addressed by limiting the dilated filter size to the input dimensions + padding across each dimension? I'd be reluctant to reduce either stride or dilation parameter to less than the size of the input tensor lest there be use cases we are unfamiliar with, because even though a typical localized convolution (e.g. particularly for image and computer vision uses, like blurring) will indeed have small strides and dilations, there may be uses cases where the convolve operator is treated more as a sparse linear operator, potentially useful for graph‑lowering (e.g. strided gathers, some sparse GEMM implementation, FIR filtering, broader neighborhood sampling, subtiling to avoid false sharing...).

Should we also have padding size limit?

Ningxin: Padding on convolution is equivalent calling the pad operator explicitly beforehand and filling with zeros. So I would set whatever limit onto the padding that the pad operator has (e.g. kernel size + pad <= valid dimension int32.max).

We should also go over other complex ops to see where we can harden the limits.

👍👀

[dsharlet]

Hi all, I have some experience with weird cases related to these parameters, I recommend including some limits I'll describe below.

First, some notation:

output_i = input_i*S_i + kernel_i*D_i

Here in dimension i, we have the output coordinate i determined by the input and kernel coordinates, and S, D indicate stride and dilation. K_i will indicate the size of the kernel reduction dimension.

Here are the typical legitimate use cases I've seen:

• Convolution/pooling: S = 1 or 2, D = 1 or 2, K in [2, 7]
• Tiled sums: S=K, D=1
• Overlapping tiled sums: S=K/2, D=1 would be 50% overlapping tiles.

There is probably in general a spectrum where very large kernels make sense as long as the stride is also large. For example, if you were going to downsample a signal by 100x, you might want a kernel with 200 samples and a stride of 100. But for a stride of 1, a kernel of 200 samples is fishy (but maybe not incorrect).

Similarly, if the stride is very large, we might be skipping some input values entirely, which is also fishy (but maybe not incorrect).

To make that intution more explicit:

• product(K) > product(S) * (some constant e.g. 100), where the product is computed over all the dimensions of the kernel.
• It might also make sense for a similar check that considers the "effective" kernel size: product(K*D) > product(S) * (some constant). This might not be arithmetically expensive, but might be trying to compute overlapping indices.
• S > D*K: this means that there are values of the input that aren't being used in the computation. This is also a really easy to way to produce huge indices for memory accesses, which would need to be checked for overflow.

These rules would catch some valid graphs, but I think it's very unlikely that "real" graphs would violate these checks, and they would catch some malicious graphs trying to make the inference engine compute huge intermediate indices/buffers.

[philloooo]

I also wonder if it makes sense to limit the input tensor element size for conv2d/pool2d to 2**16(65536) instead of int32. If it's too restrictive for the spec, we can also consider putting this in opSupportLimits similar to rankRange.
if we limit the input tensor size to more realistic values, limiting the dilated filter size to the input dimensions + padding across each dimension would be sufficient.

[fdwr]

recommend including some limits I'll describe below.

Dillion: Pondering those 🤔⏳. Thanks.

I also wonder if it makes sense to limit the input tensor element size for conv2d/pool2d to 2**16(65536) instead of int32

Phillis: Wouldn't such tiny limits would break our demos, as even Stable Diffusion's VAE decoder has typical size of 262'144 pixels (33'554'432 elements in the input, 786'432 elements in the output) in the last convolution?

So, we have to ensure that any limits we choose are well informed based on existing models, but if even existing models already exceed some of the limits being proposed, then surely there are models we don't know about that will be broken.

[philloooo]

Phillis: Wouldn't such tiny limits would break our demos, as even Stable Diffusion's VAE decoder has typical size of 262'144 elements in the last convolution?

ahh ok , so for conv2d would limiting the H, and W to 2**16 be ok?

[fdwr]

ahh ok , so for conv2d would limiting the H, and W to 2**16 be ok?

🤔 Hmm, I'm unsure about even that, because shouldn't it be legitimate to reshape 4D to 3D [N,C,H*W] where flattening the spatial dimensions could have some performance benefits for so-called 1x1 convolutions? Mind you, rather than the caller explicitly lowering it, the underlying implementations themselves should typically do these internally (like DirectML did, flattening adjacent dimensions that were contiguously packed to simplify shader calculations), but nonetheless it seems perfectly valid for this to work (and backends like DML didn't always do that, until I added it 😉).

Of course, specific API's may have per-dimension limits (and for that reason, I get adding per-dimension limits to opSupportLimits), but the API's I'm aware have the same limit for per-dimension and for total-element-count, because one can always reshape any tensor to 1D, and thus from API perspectives, a tensor of shape [1'000'000] and another of [1'000, 1'000] have the same limit.

[philloooo]

As per discussion in WG meeting, here are some examples where the intermediate calculations can overflow:
Pool2d:

• sizeof(void*) * ((pooling_size - 1) + output_height * step_height)

Conv2d:

• sizeof(void*) * kernel_size * tiled_output_size;

• sizeof(void*) * kernel_size * output_height * stride_width *
  round_up(divide_round_up(output_width, stride_width), mr)

• round_up_po2(sizeof(void*) * (primary_tile - kernel_size +
  output_height * step_height),
  XNN_ALLOCATION_ALIGNMENT)

Hmm, I'm unsure about even that, because shouldn't it be legitimate to reshape 4D to 3D [N,C,H*W] where flattening the spatial dimensions could have some performance benefits for so-called 1x1 convolutions

Would capping the product of H*W to 2**20 work?