Noticing this patch only here rather than the original post, sorry.
On Sun, Jul 16, 2023 at 09:44:32PM +0200, Greg Kroah-Hartman wrote:
From: David Howells dhowells@redhat.com
...
[ Upstream commit 86d7bd6e66e9925f0f04a7bcf3c92c05fdfefb5a ]
- o2net_hand = kzalloc(sizeof(struct o2net_handshake), GFP_KERNEL);
- o2net_keep_req = kzalloc(sizeof(struct o2net_msg), GFP_KERNEL);
- o2net_keep_resp = kzalloc(sizeof(struct o2net_msg), GFP_KERNEL);
- if (!o2net_hand || !o2net_keep_req || !o2net_keep_resp)
- folio = folio_alloc(GFP_KERNEL | __GFP_ZERO, 0);
- if (!folio) goto out;
- p = folio_address(folio);
- o2net_hand = p;
- p += sizeof(struct o2net_handshake);
- o2net_keep_req = p;
- p += sizeof(struct o2net_msg);
- o2net_keep_resp = p;
Do we care that we aren't validating sizes here? The original code allocates the structures by size. This code grabs an order 0 folio and assumes the total size of these structures is smaller than a page. But while we "know" today that the handshake messages have no payload, `o2net_msg.buf` could theoretically be filled by more data. What if someone decided to change the code to put some payload in `o2net_keep_resp.buf`? Yes, we would hope they would think to add the appropriate allocation. But should we be validating this with some kind of safety check?
Thanks, Joel