Steve French <smfrench@xxxxxxxxx> writes:
> We could move the check - but we would still have to check that the
> other offset (security buffer offset) doesn't go beyond end of SMB.
>
> It could save one line by moving this callout (the if command == SMB2_negotiate)
>
> +#ifdef CONFIG_CIFS_SMB311
> + if (shdr->Command == SMB2_NEGOTIATE)
> + clc_len += get_neg_ctxt_len(hdr, len, clc_len);
> +#endif /* SMB311 */
>
> but not sure if it is any clearer. Thoughts?
If you really want to do all the checks of get_neg_ctxt_len() then yes
there's probably no point in moving things around. My idea was to just
do return the right offsets/lenghts in case of 3.11 in
smb2_get_data_area(), no extra checks. There is some generic overlaping
and range checks in smb2_calc_size() using those results:
smb2_calc_size():
smb2_get_data_area_len(&offset, &data_length, hdr);
^^^^^^^^^^^^^^^^^^^^^ the offsets set earlier
cifs_dbg(FYI, "SMB2 data length %d offset %d\n", data_length, offset);
if (data_length > 0) {
/*
* Check to make sure that data area begins after fixed area,
* Note that last byte of the fixed area is part of data area
* for some commands, typically those with odd StructureSize,
* so we must add one to the calculation (and 4 to account for
* the size of the RFC1001 hdr.
*/
if (offset + 4 + 1 < len) {
cifs_dbg(VFS, "data area offset %d overlaps SMB2 header %d\n",
offset + 4 + 1, len);
data_length = 0;
} else {
len = 4 + offset + data_length;
}
}
but it's not as exaustive as what you added in get_neg_ctxt_len().
So ultimately it's up to you :)
I'm ok with both. I guess more debug is always better so:
Reviewed-by: Aurelien Aptel <aaptel@xxxxxxxx>
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
