New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix possible cdb printing overflow #615
Fix possible cdb printing overflow #615
Conversation
This fix looks ok, although I think there is still one minor issue - the size of the buffer in both cases (both the on-stack one and the heap one) are one byte too short, as the numbers don't take in to account the null terminating byte that sprintf writes. |
I thought that's covered with CDB_TO_BUF_SIZE with the +1 when it does "((bytes) * 3 + 1)"? |
Unless I'm missing something, the +1 in CDB_TO_BUF_SIZE gets taken up by a new line here: Line 348 in 73602a8
|
Doh, you are right. |
Reported by Chris Coulson (chrisccoulson) at: open-iscsi#613 tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here: void tcmu_cdb_print_info(struct tcmu_device *dev, const struct tcmulib_cmd *cmd, const char *info) { int i, n, bytes; char fix[CDB_FIX_SIZE], *buf; buf = fix; Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf: for (i = 0, n = 0; i < bytes; i++) { n += sprintf(buf + n, "%x ", cmd->cdb[i]); } To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large: if (bytes > CDB_FIX_SIZE) { buf = malloc(CDB_TO_BUF_SIZE(bytes)); if (!buf) { tcmu_dev_err(dev, "out of memory\n"); return; } } But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be: if (bytes > CDB_FIX_BYTES) { Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing: if (info) n += sprintf(buf + n, "%s", info); Signed-off-by: Mike Christie <mchristi@redhat.com>
5028e67
to
9c73b2d
Compare
@chrisccoulson, it should be fixed now. |
I'm assuming I got it right this time, and merging it. Just let me know if there are still issues and I will do another PR. |
Oh, sorry, yes - it looks fine now. Thanks for fixing this! |
Reported by Chris Coulson (chrisccoulson) at:
#613
tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack,
defined here:
void tcmu_cdb_print_info(struct tcmu_device *dev,
const struct tcmulib_cmd *cmd,
const char *info)
{
int i, n, bytes;
char fix[CDB_FIX_SIZE], *buf;
Further down, it uses sprintf to write some bytes in 2-digit hex format to
buf, with each byte occupying 3 bytes in buf:
To avoid overflowing the stack buffer, there is a check earlier in the
function which performs a heap allocation if bytes is too large:
But this check looks wrong. If bytes > 64, then the previously mentioned for
loop will overflow the 193 byte stack buffer. I think it should be:
Correcting this isn't sufficient on its own though, as this check doesn't
take in to account that an additional string (info) is written to buf and
can also result in the stack buffer overflowing:
Signed-off-by: Mike Christie mchristi@redhat.com