On Mon, Sep 16, 2024 at 06:14:31PM +0300, Dan Carpenter wrote:
> Hello Chuck Lever,
>
> Commit b68e4c5c3227 ("SUNRPC: Convert unwrap_integ_data() to use
> xdr_stream") from Jan 2, 2023 (linux-next), leads to the following
> Smatch static checker warning:
>
> net/sunrpc/auth_gss/svcauth_gss.c:895 svcauth_gss_unwrap_integ()
> warn: potential user controlled sizeof overflow 'offset + 4'
>
> net/sunrpc/auth_gss/svcauth_gss.c
> 859 static noinline_for_stack int
> 860 svcauth_gss_unwrap_integ(struct svc_rqst *rqstp, u32 seq, struct gss_ctx *ctx)
> 861 {
> 862 struct gss_svc_data *gsd = rqstp->rq_auth_data;
> 863 struct xdr_stream *xdr = &rqstp->rq_arg_stream;
> 864 u32 len, offset, seq_num, maj_stat;
> 865 struct xdr_buf *buf = xdr->buf;
> 866 struct xdr_buf databody_integ;
> 867 struct xdr_netobj checksum;
> 868
> 869 /* Did we already verify the signature on the original pass through? */
> 870 if (rqstp->rq_deferred)
> 871 return 0;
> 872
> 873 if (xdr_stream_decode_u32(xdr, &len) < 0)
> ^^^^
> 874 goto unwrap_failed;
> 875 if (len & 3)
>
> There used a if (len > buf->len) here but it was deleted.
True, there is no /explicit/ bounds check, but AFAICT,
xdr_buf_subsegment() will return -1 if the value of @len is larger
than the remaining space in @buf.
> 876 goto unwrap_failed;
> 877 offset = xdr_stream_pos(xdr);
> 878 if (xdr_buf_subsegment(buf, &databody_integ, offset, len))
>
> I don't see any bounds checking in here but I might have missed it
>
> 879 goto unwrap_failed;
> 880
> 881 /*
> 882 * The xdr_stream now points to the @seq_num field. The next
> 883 * XDR data item is the @arg field, which contains the clear
> 884 * text RPC program payload. The checksum, which follows the
> 885 * @arg field, is located and decoded without updating the
> 886 * xdr_stream.
> 887 */
> 888
> 889 offset += len;
> ^^^^^^^^^^^^^
> This could be an integer overflow?
Unlikely, if @len has been properly bounds-checked above. These
values will never be larger than a megabyte or two.
> 890 if (xdr_decode_word(buf, offset, &checksum.len))
> 891 goto unwrap_failed;
> 892 if (checksum.len > sizeof(gsd->gsd_scratch))
> 893 goto unwrap_failed;
> 894 checksum.data = gsd->gsd_scratch;
> --> 895 if (read_bytes_from_xdr_buf(buf, offset + XDR_UNIT, checksum.data,
> ^^^^^^^^^^^^^^^^^
> This integer overflow warning is only about foo + sizeof() to cut down on false
> positives.
>
> 896 checksum.len))
> 897 goto unwrap_failed;
> 898
> 899 maj_stat = gss_verify_mic(ctx, &databody_integ, &checksum);
> 900 if (maj_stat != GSS_S_COMPLETE)
> 901 goto bad_mic;
> 902
> 903 /* The received seqno is protected by the checksum. */
> 904 if (xdr_stream_decode_u32(xdr, &seq_num) < 0)
> 905 goto unwrap_failed;
> 906 if (seq_num != seq)
> 907 goto bad_seqno;
> 908
> 909 xdr_truncate_decode(xdr, XDR_UNIT + checksum.len);
> 910 return 0;
> 911
> 912 unwrap_failed:
> 913 trace_rpcgss_svc_unwrap_failed(rqstp);
> 914 return -EINVAL;
> 915 bad_seqno:
> 916 trace_rpcgss_svc_seqno_bad(rqstp, seq, seq_num);
> 917 return -EINVAL;
> 918 bad_mic:
> 919 trace_rpcgss_svc_mic(rqstp, maj_stat);
> 920 return -EINVAL;
> 921 }
>
> regards,
> dan carpenter
--
Chuck Lever
